D472: extensions: make wrapfunction() return a context manager
quark added a comment. In https://phab.mercurial-scm.org/D472#9064, @martinvonz wrote: > I failed with the update of this patch, so see https://phab.mercurial-scm.org/D563 instead I guess it's because of the metaedit crash. This can be updated if you have `Differential Revision: https://phab.mercurial-scm.org/D472` at the end of commit message. REPOSITORY rHG Mercurial REVISION DETAIL https://phab.mercurial-scm.org/D472 To: martinvonz, #hg-reviewers, quark, mjpieters Cc: yuja, lothiraldan, quark, mjpieters, mercurial-devel ___ Mercurial-devel mailing list Mercurial-devel@mercurial-scm.org https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
D472: extensions: make wrapfunction() return a context manager
martinvonz added a comment. I failed with the update of this patch, so see https://phab.mercurial-scm.org/D563 instead REPOSITORY rHG Mercurial REVISION DETAIL https://phab.mercurial-scm.org/D472 To: martinvonz, #hg-reviewers, quark, mjpieters Cc: yuja, lothiraldan, quark, mjpieters, mercurial-devel ___ Mercurial-devel mailing list Mercurial-devel@mercurial-scm.org https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
D472: extensions: make wrapfunction() return a context manager
yuja added a comment. This is off-topic, but if `wrapfunction()` is a context manager, I think `unwrapfunction()` could also be a context manager. INLINE COMMENTS > quark wrote in extensions.py:424 > I think using the return value of wrapfunction could lead to hidden bugs: > > oldfunc = wrapfunction(x, 'y', newfunc) > ... > # what if somebody else wraps x.y here? > ... > x.y = oldfunc > > So it seems reasonable to me to deprecate that and migrate users to > `unwrapfunction`. My point is why do we care only for `classobj.method = oldfunc`, which seems less likely than `oldfunc()`. REPOSITORY rHG Mercurial REVISION DETAIL https://phab.mercurial-scm.org/D472 To: martinvonz, #hg-reviewers, quark, mjpieters Cc: yuja, lothiraldan, quark, mjpieters, mercurial-devel ___ Mercurial-devel mailing list Mercurial-devel@mercurial-scm.org https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
D472: extensions: make wrapfunction() return a context manager
quark added inline comments. INLINE COMMENTS > yuja wrote in extensions.py:424 > I see, but I don't think it's more common than wrapping a plain > function. > > If the compatibility matters, we probably shouldn't replace > `wrapfunction` with a context manager. I think using the return value of wrapfunction could lead to hidden bugs: oldfunc = wrapfunction(x, 'y', newfunc) ... # what if somebody else wraps x.y here? ... x.y = oldfunc So it seems reasonable to me to deprecate that and migrate users to `unwrapfunction`. REPOSITORY rHG Mercurial REVISION DETAIL https://phab.mercurial-scm.org/D472 To: martinvonz, #hg-reviewers, quark, mjpieters Cc: yuja, lothiraldan, quark, mjpieters, mercurial-devel ___ Mercurial-devel mailing list Mercurial-devel@mercurial-scm.org https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
D472: extensions: make wrapfunction() return a context manager
yuja added inline comments. INLINE COMMENTS > martinvonz wrote in extensions.py:424 > See commit message. It's for compatibility with existing callers that bind > the returned value. I see, but I don't think it's more common than wrapping a plain function. If the compatibility matters, we probably shouldn't replace `wrapfunction` with a context manager. REPOSITORY rHG Mercurial REVISION DETAIL https://phab.mercurial-scm.org/D472 To: martinvonz, #hg-reviewers, quark, mjpieters Cc: yuja, lothiraldan, quark, mjpieters, mercurial-devel ___ Mercurial-devel mailing list Mercurial-devel@mercurial-scm.org https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
D472: extensions: make wrapfunction() return a context manager
yuja added inline comments.
INLINE COMMENTS
> extensions.py:424
> +
> +def __get__(self, instance, owner):
> +util.nouideprecwarn("Using the return value of wrapfunction() as a "
When will this `__get__()` be called?
`wrapfunction()` is generally used for wrapping a free function,
not an unbound method.
REPOSITORY
rHG Mercurial
REVISION DETAIL
https://phab.mercurial-scm.org/D472
To: martinvonz, #hg-reviewers, quark, mjpieters
Cc: yuja, lothiraldan, quark, mjpieters, mercurial-devel
___
Mercurial-devel mailing list
Mercurial-devel@mercurial-scm.org
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
D472: extensions: make wrapfunction() return a context manager
mjpieters accepted this revision. mjpieters added a comment. I was being too strict; like `open()` it's sometimes fine to apply the action in the constructor. REPOSITORY rHG Mercurial REVISION DETAIL https://phab.mercurial-scm.org/D472 To: martinvonz, #hg-reviewers, quark, mjpieters Cc: lothiraldan, quark, mjpieters, mercurial-devel ___ Mercurial-devel mailing list Mercurial-devel@mercurial-scm.org https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
D472: extensions: make wrapfunction() return a context manager
martinvonz added a comment. In https://phab.mercurial-scm.org/D472#7758, @quark wrote: > In https://phab.mercurial-scm.org/D472#7756, @lothiraldan wrote: > > > All users of wrapfunction will need to be updated anyway to use the context manager style > > > I think only those who use the return value of wrapfunction will need change (actually, new code is more correct regarding on nested wrapfunction handling) and expect that to be not too many. If wrapfunction is used to replace a function permanently, it does not need change. > > Reusing name is more consistent with `repo.transaction`, `repo.lock` and `open`. I personally don't care much which way we go here. I originally did make it a separate function (wrappedfunction()) and did the wrapping in __enter__, but I'm fine either way. REPOSITORY rHG Mercurial REVISION DETAIL https://phab.mercurial-scm.org/D472 To: martinvonz, #hg-reviewers, quark, mjpieters Cc: lothiraldan, quark, mjpieters, mercurial-devel ___ Mercurial-devel mailing list Mercurial-devel@mercurial-scm.org https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
D472: extensions: make wrapfunction() return a context manager
quark added a comment. In https://phab.mercurial-scm.org/D472#7756, @lothiraldan wrote: > All users of wrapfunction will need to be updated anyway to use the context manager style I think only those who use the return value of wrapfunction will need change (actually, new code is more correct regarding on nested wrapfunction handling) and expect that to be not too many. If wrapfunction is used to replace a function permanently, it does not need change. REPOSITORY rHG Mercurial REVISION DETAIL https://phab.mercurial-scm.org/D472 To: martinvonz, #hg-reviewers, quark, mjpieters Cc: lothiraldan, quark, mjpieters, mercurial-devel ___ Mercurial-devel mailing list Mercurial-devel@mercurial-scm.org https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
D472: extensions: make wrapfunction() return a context manager
lothiraldan added a comment. Maybe instead of replacing wrapfunction with a context manager, what about introducing it with a separate name? All users of wrapfunction will need to be updated anyway to use the context manager style REPOSITORY rHG Mercurial REVISION DETAIL https://phab.mercurial-scm.org/D472 To: martinvonz, #hg-reviewers, quark, mjpieters Cc: lothiraldan, quark, mjpieters, mercurial-devel ___ Mercurial-devel mailing list Mercurial-devel@mercurial-scm.org https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
D472: extensions: make wrapfunction() return a context manager
quark added inline comments. INLINE COMMENTS > mjpieters wrote in extensions.py:408-412 > Move this to `__enter__`. You want to be able to create the context manager > separately from actually entering the context: > > cmwrapper = wrapfunction(cont, 'name', wrapper) > > # do other things, `cont.name` is *not* wrapped > > with cmwrapper: > # `cont.name` is wrapped The point is `wrapfunction` is also intended to work with non-context case: wrapfunction(x, y, z) # works and should have immediate side effect REPOSITORY rHG Mercurial REVISION DETAIL https://phab.mercurial-scm.org/D472 To: martinvonz, #hg-reviewers, quark, mjpieters Cc: quark, mjpieters, mercurial-devel ___ Mercurial-devel mailing list Mercurial-devel@mercurial-scm.org https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
D472: extensions: make wrapfunction() return a context manager
mjpieters added inline comments. INLINE COMMENTS > extensions.py:408-412 > +origfn = getattr(container, funcname) > +assert callable(origfn) > +wrap = bind(wrapper, origfn) > +_updatewrapper(wrap, origfn, wrapper) > +setattr(container, funcname, wrap) Move this to `__enter__`. You want to be able to create the context manager separately from actually entering the context: cmwrapper = wrapfunction(cont, 'name', wrapper) # do other things, `cont.name` is *not* wrapped with cmwrapper: # `cont.name` is wrapped REPOSITORY rHG Mercurial REVISION DETAIL https://phab.mercurial-scm.org/D472 To: martinvonz, #hg-reviewers, quark Cc: quark, mjpieters, mercurial-devel ___ Mercurial-devel mailing list Mercurial-devel@mercurial-scm.org https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
D472: extensions: make wrapfunction() return a context manager
quark added subscribers: mjpieters, quark. quark accepted this revision. quark added a comment. cc python expert @mjpieters to see if he has additional comments. But I think this is good enough. REPOSITORY rHG Mercurial REVISION DETAIL https://phab.mercurial-scm.org/D472 To: martinvonz, #hg-reviewers, quark Cc: quark, mjpieters, mercurial-devel ___ Mercurial-devel mailing list Mercurial-devel@mercurial-scm.org https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
D472: extensions: make wrapfunction() return a context manager
martinvonz created this revision.
Herald added a subscriber: mercurial-devel.
Herald added a reviewer: hg-reviewers.
REVISION SUMMARY
Several extensions exist that temporarily want to wrap a function (at
least narrowhg, any many of the extensions in hg-experimental). That's
why we have the unwrapfunction() that was introduced in
https://phab.mercurial-scm.org/rHG19578bb847310e32eae3d44466b51a698ced5b95
(extensions: add unwrapfunction to undo wrapfunction, 2016-08-10).
This patch makes wrapfunction() return a context manager. Since
wrapfunction() returned the original function before this patch, we
will be changing the return type. In order to limit breakages for
callers that kept the reference to the unbound method around and then
set it back on the container after it was done, this patch also
implements a __get__ method that binds the original method to the
container instance.
REPOSITORY
rHG Mercurial
REVISION DETAIL
https://phab.mercurial-scm.org/D472
AFFECTED FILES
mercurial/extensions.py
tests/test-extensions-wrapfunction.py
tests/test-extensions-wrapfunction.py.out
tests/test-filecache.py
CHANGE DETAILS
diff --git a/tests/test-filecache.py b/tests/test-filecache.py
--- a/tests/test-filecache.py
+++ b/tests/test-filecache.py
@@ -129,20 +129,16 @@
def wrapinit(orig, *args, **kwargs):
pass
-originit = extensions.wrapfunction(util.cachestat, '__init__', wrapinit)
-origcacheable = extensions.wrapfunction(util.cachestat, 'cacheable',
-wrapcacheable)
+with extensions.wrapfunction(util.cachestat, '__init__', wrapinit),\
+ extensions.wrapfunction(util.cachestat, 'cacheable', wrapcacheable):
-for fn in ['x', 'y']:
-try:
-os.remove(fn)
-except OSError:
-pass
+for fn in ['x', 'y']:
+try:
+os.remove(fn)
+except OSError:
+pass
-basic(fakerepo())
-
-util.cachestat.cacheable = origcacheable
-util.cachestat.__init__ = originit
+basic(fakerepo())
def test_filecache_synced():
# test old behavior that caused filecached properties to go out of sync
diff --git a/tests/test-extensions-wrapfunction.py.out
b/tests/test-extensions-wrapfunction.py.out
--- a/tests/test-extensions-wrapfunction.py.out
+++ b/tests/test-extensions-wrapfunction.py.out
@@ -12,3 +12,8 @@
unwrap 2: 2: [1, 'orig']
unwrap 1: 1: ['orig']
unwrap -: -: IndexError
+context manager ['orig']
+context manager [0, 'orig']
+context manager [1, 0, 'orig']
+context manager [0, 'orig']
+context manager ['orig']
diff --git a/tests/test-extensions-wrapfunction.py
b/tests/test-extensions-wrapfunction.py
--- a/tests/test-extensions-wrapfunction.py
+++ b/tests/test-extensions-wrapfunction.py
@@ -37,3 +37,11 @@
batchwrap(wrappers + [wrappers[0]])
batchunwrap([(wrappers[i] if i >= 0 else None)
for i in [3, None, 0, 4, 0, 2, 1, None]])
+
+print('context manager', dummy.getstack())
+with extensions.wrapfunction(dummy, 'getstack', wrappers[0]):
+print('context manager', dummy.getstack())
+with extensions.wrapfunction(dummy, 'getstack', wrappers[1]):
+print('context manager', dummy.getstack())
+print('context manager', dummy.getstack())
+print('context manager', dummy.getstack())
diff --git a/mercurial/extensions.py b/mercurial/extensions.py
--- a/mercurial/extensions.py
+++ b/mercurial/extensions.py
@@ -399,6 +399,34 @@
raise AttributeError(r"type '%s' has no property '%s'" % (
cls, propname))
+class _wrappedfunction(object):
+'''context manager for temporarily wrapping a function'''
+
+def __init__(self, container, funcname, wrapper):
+assert callable(wrapper)
+
+origfn = getattr(container, funcname)
+assert callable(origfn)
+wrap = bind(wrapper, origfn)
+_updatewrapper(wrap, origfn, wrapper)
+setattr(container, funcname, wrap)
+
+self._container = container
+self._funcname = funcname
+self._origfn = origfn
+
+def __enter__(self):
+return self
+
+def __exit__(self, exctype, excvalue, traceback):
+unwrapfunction(self._container, self._funcname)
+
+def __get__(self, instance, owner):
+util.nouideprecwarn("Using the return value of wrapfunction() as a "
+"function is deprecated. Use it as context manager
"
+"instead","4.4", stacklevel=2)
+return bind(self._origfn, instance)
+
def wrapfunction(container, funcname, wrapper):
'''Wrap the function named funcname in container
@@ -432,14 +460,7 @@
your end users, you should play nicely with others by using the
subclass trick.
'''
-assert callable(wrapper)
-
-origfn = getattr(container, funcname)
-assert callable(origfn)
-wrap = bind(wrapper, origfn)
-_updatewrapper(wrap, origfn, wrapper)
-
