| Submitter | Laurent Charignon |
|---|---|
| Date | Jan. 14, 2016, 10:17 p.m. |
| Message ID | <c1ba4521aea0906874fe.1452809868@lcharignon-mbp.dhcp.thefacebook.com> |
| Download | mbox | patch |
| Permalink | /patch/12776/ |
| State | Changes Requested |
| Delegated to: | Pierre-Yves David |
| Headers | show |
Comments
On Thu, Jan 14, 2016 at 2:17 PM, Laurent Charignon <lcharignon@fb.com> wrote: > # HG changeset patch > # User Laurent Charignon <lcharignon@fb.com> > # Date 1452809804 28800 > # Thu Jan 14 14:16:44 2016 -0800 > # Node ID c1ba4521aea0906874fe7de5bee156d5070c0e56 > # Parent e6e34c4e391668c5d8af8f98c004a27c77b1e2fa > extensions: add get contextmanager to avoid common pattern > > Before this patch, people were sometimes using the following pattern: > > try: > m = extensions.find(name) > ... > except KeyError: > pass > > This patch simplifies the pattern with context managers as follow: > > with extensions.get(name) as m: > ... > > diff --git a/mercurial/extensions.py b/mercurial/extensions.py > --- a/mercurial/extensions.py > +++ b/mercurial/extensions.py > @@ -7,6 +7,7 @@ > > from __future__ import absolute_import > > +from contextlib import contextmanager > import imp > import os > > @@ -41,6 +42,13 @@ def extensions(ui=None): > if module and enabled(name): > yield name, module > > +@contextmanager > +def get(name): > + try: > + yield find(name) > + except KeyError: > + pass > + > Uh, I don't think this is valid. @contextmanager expects *something* to be returned: import contextlib @contextlib.contextmanager def cm(): pass with cm() as m: print('hello') $ python test.py Traceback (most recent call last): File "test.py", line 7, in <module> with cm() as m: File "/usr/local/Cellar/python/2.7.11/Frameworks/Python.framework/Versions/2.7/lib/python2.7/contextlib.py", line 17, in __enter__ return self.gen.next() AttributeError: 'NoneType' object has no attribute 'next' Even if it did work, it's not like the code inside the context manager in the next patch wouldn't get executed - it would just have None instead of the extension object. Furthermore, context managers don't feel like the right fit here. Generic replacement for many try..finally, yes. For try..except, not really.
On Thu, 14 Jan 2016 14:45:47 -0800, Gregory Szorc wrote: > On Thu, Jan 14, 2016 at 2:17 PM, Laurent Charignon <lcharignon@fb.com> > > # HG changeset patch > > # User Laurent Charignon <lcharignon@fb.com> > > # Date 1452809804 28800 > > # Thu Jan 14 14:16:44 2016 -0800 > > # Node ID c1ba4521aea0906874fe7de5bee156d5070c0e56 > > # Parent e6e34c4e391668c5d8af8f98c004a27c77b1e2fa > > extensions: add get contextmanager to avoid common pattern > > > > Before this patch, people were sometimes using the following pattern: > > > > try: > > m = extensions.find(name) > > ... > > except KeyError: > > pass > > > > This patch simplifies the pattern with context managers as follow: > > > > with extensions.get(name) as m: > > ... > Furthermore, context managers don't feel like the right fit here. Generic > replacement for many try..finally, yes. For try..except, not really. I agree with Gregory. Because it had no cleanup process, using context manager would just introduce extra nested block and prevent early return. If you think KeyError is ugly, how about making it return None? m = extensions.get(name) if not m: return
On 01/15/2016 07:14 AM, Yuya Nishihara wrote: > On Thu, 14 Jan 2016 14:45:47 -0800, Gregory Szorc wrote: >> On Thu, Jan 14, 2016 at 2:17 PM, Laurent Charignon <lcharignon@fb.com> >>> # HG changeset patch >>> # User Laurent Charignon <lcharignon@fb.com> >>> # Date 1452809804 28800 >>> # Thu Jan 14 14:16:44 2016 -0800 >>> # Node ID c1ba4521aea0906874fe7de5bee156d5070c0e56 >>> # Parent e6e34c4e391668c5d8af8f98c004a27c77b1e2fa >>> extensions: add get contextmanager to avoid common pattern >>> >>> Before this patch, people were sometimes using the following pattern: >>> >>> try: >>> m = extensions.find(name) >>> ... >>> except KeyError: >>> pass >>> >>> This patch simplifies the pattern with context managers as follow: >>> >>> with extensions.get(name) as m: >>> ... > >> Furthermore, context managers don't feel like the right fit here. Generic >> replacement for many try..finally, yes. For try..except, not really. > > I agree with Gregory. Because it had no cleanup process, using context manager > would just introduce extra nested block and prevent early return. > > If you think KeyError is ugly, how about making it return None? > > m = extensions.get(name) > if not m: > return I'm +1 for a None version. In my opinion, standard Code flow based on KeyError should be avoided when possible.
Patch
diff --git a/mercurial/extensions.py b/mercurial/extensions.py --- a/mercurial/extensions.py +++ b/mercurial/extensions.py @@ -7,6 +7,7 @@ from __future__ import absolute_import +from contextlib import contextmanager import imp import os @@ -41,6 +42,13 @@ def extensions(ui=None): if module and enabled(name): yield name, module +@contextmanager +def get(name): + try: + yield find(name) + except KeyError: + pass + def find(name): '''return module with given extension name''' mod = None
# HG changeset patch # User Laurent Charignon <lcharignon@fb.com> # Date 1452809804 28800 # Thu Jan 14 14:16:44 2016 -0800 # Node ID c1ba4521aea0906874fe7de5bee156d5070c0e56 # Parent e6e34c4e391668c5d8af8f98c004a27c77b1e2fa extensions: add get contextmanager to avoid common pattern Before this patch, people were sometimes using the following pattern: try: m = extensions.find(name) ... except KeyError: pass This patch simplifies the pattern with context managers as follow: with extensions.get(name) as m: ...