D5994: cleanup: prefer nested context managers to \-continuations

Previous Topic Next Topic
 
classic Classic list List threaded Threaded
4 messages Options
Reply | Threaded
Open this post in threaded view
|

D5994: cleanup: prefer nested context managers to \-continuations

pulkit (Pulkit Goyal)
durin42 created this revision.
Herald added a reviewer: martinvonz.
Herald added a subscriber: mercurial-devel.
Herald added a reviewer: hg-reviewers.

REVISION SUMMARY
  I'd prefer Python accept a tuple of context managers, but alas it
  isn't meant to be. This will have to suffice.

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D5994

AFFECTED FILES
  hgext/largefiles/overrides.py
  hgext/narrow/narrowcommands.py
  mercurial/exchange.py

CHANGE DETAILS

diff --git a/mercurial/exchange.py b/mercurial/exchange.py
--- a/mercurial/exchange.py
+++ b/mercurial/exchange.py
@@ -556,18 +556,18 @@
                % stringutil.forcebytestr(err))
         pushop.ui.debug(msg)
 
-    with wlock or util.nullcontextmanager(), \
-            lock or util.nullcontextmanager(), \
-            pushop.trmanager or util.nullcontextmanager():
-        pushop.repo.checkpush(pushop)
-        _checkpublish(pushop)
-        _pushdiscovery(pushop)
-        if not _forcebundle1(pushop):
-            _pushbundle2(pushop)
-        _pushchangeset(pushop)
-        _pushsyncphase(pushop)
-        _pushobsolete(pushop)
-        _pushbookmark(pushop)
+    with wlock or util.nullcontextmanager():
+        with lock or util.nullcontextmanager():
+            with pushop.trmanager or util.nullcontextmanager():
+                pushop.repo.checkpush(pushop)
+                _checkpublish(pushop)
+                _pushdiscovery(pushop)
+                if not _forcebundle1(pushop):
+                    _pushbundle2(pushop)
+                _pushchangeset(pushop)
+                _pushsyncphase(pushop)
+                _pushobsolete(pushop)
+                _pushbookmark(pushop)
 
     if repo.ui.configbool('experimental', 'remotenames'):
         logexchange.pullremotenames(repo, remote)
diff --git a/hgext/narrow/narrowcommands.py b/hgext/narrow/narrowcommands.py
--- a/hgext/narrow/narrowcommands.py
+++ b/hgext/narrow/narrowcommands.py
@@ -278,9 +278,9 @@
             p1, p2 = ds.p1(), ds.p2()
             with ds.parentchange():
                 ds.setparents(node.nullid, node.nullid)
-            with wrappedextraprepare,\
-                 repo.ui.configoverride(overrides, 'widen'):
-                exchange.pull(repo, remote, heads=common)
+            with wrappedextraprepare:
+                with repo.ui.configoverride(overrides, 'widen'):
+                    exchange.pull(repo, remote, heads=common)
             with ds.parentchange():
                 ds.setparents(p1, p2)
         else:
@@ -296,11 +296,11 @@
                     'ellipses': False,
                 }).result()
 
-            with repo.transaction('widening') as tr,\
-                 repo.ui.configoverride(overrides, 'widen'):
-                tgetter = lambda: tr
-                bundle2.processbundle(repo, bundle,
-                        transactiongetter=tgetter)
+            with repo.transaction('widening') as tr:
+                with repo.ui.configoverride(overrides, 'widen'):
+                    tgetter = lambda: tr
+                    bundle2.processbundle(repo, bundle,
+                            transactiongetter=tgetter)
 
         with repo.transaction('widening'):
             repo.setnewnarrowpats()
diff --git a/hgext/largefiles/overrides.py b/hgext/largefiles/overrides.py
--- a/hgext/largefiles/overrides.py
+++ b/hgext/largefiles/overrides.py
@@ -664,9 +664,9 @@
                                   _('destination largefile already exists'))
             copiedfiles.append((src, dest))
             orig(src, dest, *args, **kwargs)
-        with extensions.wrappedfunction(util, 'copyfile', overridecopyfile), \
-             extensions.wrappedfunction(scmutil, 'match', overridematch):
-            result += orig(ui, repo, listpats, opts, rename)
+        with extensions.wrappedfunction(util, 'copyfile', overridecopyfile):
+            with extensions.wrappedfunction(scmutil, 'match', overridematch):
+                result += orig(ui, repo, listpats, opts, rename)
 
         lfdirstate = lfutil.openlfdirstate(ui, repo)
         for (src, dest) in copiedfiles:



To: durin42, martinvonz, #hg-reviewers
Cc: mercurial-devel
_______________________________________________
Mercurial-devel mailing list
[hidden email]
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
Reply | Threaded
Open this post in threaded view
|

D5994: cleanup: prefer nested context managers to \-continuations

pulkit (Pulkit Goyal)
martinvonz added a comment.


  > I'd prefer Python accept a tuple of context managers
 
  I think I'd also prefer if our context managers didn't acquire the resource in `__init__`. That would let us write it with less indentation as:
 
    wlock = wlock or util.nullcontextmanager()
    lock = lock or util.nullcontextmanager()
    trmanager = pushop.trmanager or util.nullcontextmanager()
    with wlock, lock, trmanager:
 
  (In this case we probably could do that anyway since we don't seem to worry about the risk of exceptions from the time we take the lock until we enter the context manager.)
 
  I don't care much either way about the patch itself. I'll wait a little to see if someone else cares more, but otherwise I'll take.

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D5994

To: durin42, martinvonz, #hg-reviewers
Cc: mercurial-devel
_______________________________________________
Mercurial-devel mailing list
[hidden email]
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
Reply | Threaded
Open this post in threaded view
|

D5994: cleanup: prefer nested context managers to \-continuations

pulkit (Pulkit Goyal)
In reply to this post by pulkit (Pulkit Goyal)
durin42 added a comment.


  In https://phab.mercurial-scm.org/D5994#87506, @martinvonz wrote:
 
  > > I'd prefer Python accept a tuple of context managers
  >
  > I think I'd also prefer if our context managers didn't acquire the resource in `__init__`. That would let us write it with less indentation as:
  >
  >   wlock = wlock or util.nullcontextmanager()
  >   lock = lock or util.nullcontextmanager()
  >   trmanager = pushop.trmanager or util.nullcontextmanager()
  >   with wlock, lock, trmanager:
  >
  >
  > (In this case we probably could do that anyway since we don't seem to worry about the risk of exceptions from the time we take the lock until we enter the context manager.)
  >
  > I don't care much either way about the patch itself. I'll wait a little to see if someone else cares more, but otherwise I'll take.
 
 
  That sounds like a good cleanup anyway - probably taking the resource is a vestige of when we were on around Python 2.3 and mpm had this discipline of using `del` to free things like locks. It worked, but isn't idiomatic today (I'm honestly not sure if it was then - but mpm is where I learned it.)

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D5994

To: durin42, martinvonz, #hg-reviewers
Cc: mercurial-devel
_______________________________________________
Mercurial-devel mailing list
[hidden email]
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
Reply | Threaded
Open this post in threaded view
|

D5994: cleanup: prefer nested context managers to \-continuations

pulkit (Pulkit Goyal)
In reply to this post by pulkit (Pulkit Goyal)
This revision was automatically updated to reflect the committed changes.
Closed by commit rHG1eb2fc21da12: cleanup: prefer nested context managers to \-continuations (authored by durin42, committed by ).

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST UPDATE
  https://phab.mercurial-scm.org/D5994?vs=14164&id=14179

REVISION DETAIL
  https://phab.mercurial-scm.org/D5994

AFFECTED FILES
  hgext/largefiles/overrides.py
  hgext/narrow/narrowcommands.py
  mercurial/exchange.py

CHANGE DETAILS

diff --git a/mercurial/exchange.py b/mercurial/exchange.py
--- a/mercurial/exchange.py
+++ b/mercurial/exchange.py
@@ -556,18 +556,18 @@
                % stringutil.forcebytestr(err))
         pushop.ui.debug(msg)
 
-    with wlock or util.nullcontextmanager(), \
-            lock or util.nullcontextmanager(), \
-            pushop.trmanager or util.nullcontextmanager():
-        pushop.repo.checkpush(pushop)
-        _checkpublish(pushop)
-        _pushdiscovery(pushop)
-        if not _forcebundle1(pushop):
-            _pushbundle2(pushop)
-        _pushchangeset(pushop)
-        _pushsyncphase(pushop)
-        _pushobsolete(pushop)
-        _pushbookmark(pushop)
+    with wlock or util.nullcontextmanager():
+        with lock or util.nullcontextmanager():
+            with pushop.trmanager or util.nullcontextmanager():
+                pushop.repo.checkpush(pushop)
+                _checkpublish(pushop)
+                _pushdiscovery(pushop)
+                if not _forcebundle1(pushop):
+                    _pushbundle2(pushop)
+                _pushchangeset(pushop)
+                _pushsyncphase(pushop)
+                _pushobsolete(pushop)
+                _pushbookmark(pushop)
 
     if repo.ui.configbool('experimental', 'remotenames'):
         logexchange.pullremotenames(repo, remote)
diff --git a/hgext/narrow/narrowcommands.py b/hgext/narrow/narrowcommands.py
--- a/hgext/narrow/narrowcommands.py
+++ b/hgext/narrow/narrowcommands.py
@@ -278,9 +278,9 @@
             p1, p2 = ds.p1(), ds.p2()
             with ds.parentchange():
                 ds.setparents(node.nullid, node.nullid)
-            with wrappedextraprepare,\
-                 repo.ui.configoverride(overrides, 'widen'):
-                exchange.pull(repo, remote, heads=common)
+            with wrappedextraprepare:
+                with repo.ui.configoverride(overrides, 'widen'):
+                    exchange.pull(repo, remote, heads=common)
             with ds.parentchange():
                 ds.setparents(p1, p2)
         else:
@@ -296,11 +296,11 @@
                     'ellipses': False,
                 }).result()
 
-            with repo.transaction('widening') as tr,\
-                 repo.ui.configoverride(overrides, 'widen'):
-                tgetter = lambda: tr
-                bundle2.processbundle(repo, bundle,
-                        transactiongetter=tgetter)
+            with repo.transaction('widening') as tr:
+                with repo.ui.configoverride(overrides, 'widen'):
+                    tgetter = lambda: tr
+                    bundle2.processbundle(repo, bundle,
+                            transactiongetter=tgetter)
 
         with repo.transaction('widening'):
             repo.setnewnarrowpats()
diff --git a/hgext/largefiles/overrides.py b/hgext/largefiles/overrides.py
--- a/hgext/largefiles/overrides.py
+++ b/hgext/largefiles/overrides.py
@@ -664,9 +664,9 @@
                                   _('destination largefile already exists'))
             copiedfiles.append((src, dest))
             orig(src, dest, *args, **kwargs)
-        with extensions.wrappedfunction(util, 'copyfile', overridecopyfile), \
-             extensions.wrappedfunction(scmutil, 'match', overridematch):
-            result += orig(ui, repo, listpats, opts, rename)
+        with extensions.wrappedfunction(util, 'copyfile', overridecopyfile):
+            with extensions.wrappedfunction(scmutil, 'match', overridematch):
+                result += orig(ui, repo, listpats, opts, rename)
 
         lfdirstate = lfutil.openlfdirstate(ui, repo)
         for (src, dest) in copiedfiles:



To: durin42, martinvonz, #hg-reviewers
Cc: mercurial-devel
_______________________________________________
Mercurial-devel mailing list
[hidden email]
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel