Officially deprecate Transaction.begin().

NEWS.txt

@@ -5,14 +5,14 @@ Release date: DD-MMM-YYYY
 Connection
 ----------
 
-ZODB intends to raise ConnnectionStateError if an attempt is made to
-close a connection while modifications are pending (the connection is
-involved in a transaction that hasn't been abort()'ed or commit()'ed).
-It was missing the case where the only pending modifications were made
-in subtransactions.  This has been fixed.  If an attempt to close a
-connection with pending subtransactions is made now,
+ZODB intends to raise ConnnectionStateError if an attempt is made to close
+a connection while modifications are pending (the connection is involved in
+a transaction that hasn't been ``abort()``'ed or ``commit()``'ed). It was
+missing the case where the only pending modifications were made in
+subtransactions.  This has been fixed.  If an attempt to close a connection
+with pending subtransactions is made now::
 
-    ``ConnnectionStateError: Cannot close a connection with a pending subtransaction``
+    ConnnectionStateError: Cannot close a connection with a pending subtransaction
 
 is raised.
 
@@ -29,21 +29,38 @@ transaction
   that ZODB intends to prevent committing a transaction in which a
   ReadConflictError occurred; this was an obscure case it missed.
 
-- Growing pains:  ZODB 3.1 and 3.2 had a bug wherein ``Transaction.begin()``
-  didn't abort the current transaction if the only pending changes were in a
-  subtransaction.  In ZODB 3.3, it's intended that transaction managers be
-  used instead of invoking methods directly on Transaction objects, and
-  calling ``begin()`` on a transaction manager didn't have this old bug.
-  However, ``Transaction.begin()`` still exists in 3.3, and it had a worse
-  bug:  it never aborted the transaction (not even if changes were pending
-  outside of subtransactions).  ``Transaction.begin()`` has been changed to
-  abort the transaction, although it's still strongly recommended to invoke
-  ``begin()`` on the relevant transaction manager instead.  For example::
-
-    import transaction
-    transaction.begin()
+- Growing pains:  ZODB 3.2 had a bug wherein ``Transaction.begin()`` didn't
+  abort the current transaction if the only pending changes were in a
+  subtransaction.  In ZODB 3.3, it's intended that a transaction manager be
+  used to effect ``begin()`` (instead of invoking ``Transaction.begin()``),
+  and calling ``begin()`` on a transaction manager didn't have this old
+  bug.  However, ``Transaction.begin()`` still exists in 3.3, and it had a
+  worse bug:  it never aborted the transaction (not even if changes were
+  pending outside of subtransactions). ``Transaction.begin()`` has been
+  changed to abort the transaction. ``Transaction.begin()`` is also
+  deprecated.  Don't use it.  Use ``begin()`` on the relevant transaction
+  manager instead.  For example,
+
+      >>> import transaction
+      >>> txn = transaction.begin()  # start a txn using the default TM
 
   if using the default ThreadTransactionManager (see news for 3.3a3 below).
+  In 3.3, it's intended that a single Transaction object is used for exactly
+  one transaction.  So, unlike as in 3.2, when somtimes Transaction objects
+  were reused across transactions, but sometimes weren't, when you do
+  ``Transaction.begin()`` in 3.3 a brand new transaction object is
+  created.  That's why this use is deprecated.  Code of the form:
+
+      >>> txn = transaction.get()
+      >>> ...
+      >>> txn.begin()
+      >>> ...
+      >>> txn.commit()
+
+  can't work as intended is 3.3, because ``txn`` is no longer the current
+  Transaction object the instant ``txn.begin()`` returns.
+
+
 
 BTrees
 ------

src/ZODB/tests/testZODB.py

@@ -386,38 +386,57 @@ class ZODBTests(unittest.TestCase):
         # transaction, and, in fact, when this test was written,
         # Transaction.begin() didn't do anything (everything from here
         # down failed).
-        cn = self._db.open()
-        rt = cn.root()
-        rt['a'] = 1
-
-        transaction.get().begin()  # should abort adding 'a' to the root
-        rt = cn.root()
-        self.assertRaises(KeyError, rt.__getitem__, 'a')
-
-        # A longstanding bug:  this didn't work if changes were only in
-        # subtransactions.
-        transaction.get().begin()
-        rt = cn.root()
-        rt['a'] = 2
-        transaction.get().commit(1)
 
-        transaction.get().begin()
-        rt = cn.root()
-        self.assertRaises(KeyError, rt.__getitem__, 'a')
+        # Oh, bleech.  Since Transaction.begin is also deprecated, we have
+        # to goof around suppressing the deprecation warning.
+        import warnings
 
-        # One more time, mixing "top level" and subtransaction changes.
-        transaction.get().begin()
-        rt = cn.root()
-        rt['a'] = 3
-        transaction.get().commit(1)
-        rt['b'] = 4
+        # First verify that Transaction.begin *is* deprecated, by turning
+        # the warning into an error.
+        warnings.filterwarnings("error", category=DeprecationWarning)
+        self.assertRaises(DeprecationWarning, transaction.get().begin)
+        del warnings.filters[0]
 
-        transaction.get().begin()
-        rt = cn.root()
-        self.assertRaises(KeyError, rt.__getitem__, 'a')
-        self.assertRaises(KeyError, rt.__getitem__, 'b')
+        # Now ignore DeprecationWarnings for the duration.  Use a
+        # try/finally block to ensure we reenable DeprecationWarnings
+        # no matter what.
+        warnings.filterwarnings("ignore", category=DeprecationWarning)
+        try:
+            cn = self._db.open()
+            rt = cn.root()
+            rt['a'] = 1
+
+            transaction.get().begin()  # should abort adding 'a' to the root
+            rt = cn.root()
+            self.assertRaises(KeyError, rt.__getitem__, 'a')
+
+            # A longstanding bug:  this didn't work if changes were only in
+            # subtransactions.
+            transaction.get().begin()
+            rt = cn.root()
+            rt['a'] = 2
+            transaction.get().commit(1)
+
+            transaction.get().begin()
+            rt = cn.root()
+            self.assertRaises(KeyError, rt.__getitem__, 'a')
+
+            # One more time, mixing "top level" and subtransaction changes.
+            transaction.get().begin()
+            rt = cn.root()
+            rt['a'] = 3
+            transaction.get().commit(1)
+            rt['b'] = 4
+
+            transaction.get().begin()
+            rt = cn.root()
+            self.assertRaises(KeyError, rt.__getitem__, 'a')
+            self.assertRaises(KeyError, rt.__getitem__, 'b')
+
+            cn.close()
 
-        cn.close()
+        finally:
+            del warnings.filters[0]
 
 def test_suite():
     return unittest.makeSuite(ZODBTests, 'check')

src/transaction/_transaction.py

@@ -136,6 +136,7 @@ XXX This code isn't tested.
 import logging
 import sys
 import thread
+import warnings
 
 _marker = object()
 
@@ -230,6 +231,9 @@ class Transaction(object):
                 self._resources.append(adapter)
 
     def begin(self):
+        warnings.warn("Transaction.begin() should no longer be used; use "
+                      "the begin() method of a transaction manager.",
+                      DeprecationWarning)
         if (self._resources or
               self._sub or
               self._nonsub or