This is an archive of the discontinued Mercurial Phabricator instance.

Differential D472

extensions: add wrappedfunction() context manager
ClosedPublic

Authored by martinvonz on Aug 22 2017, 2:29 AM.

Details

Reviewers
quark
mjpieters
yuja
Group Reviewers
hg-reviewers
Commits
rHG47e52f079a57: extensions: add wrappedfunction() context manager
rHG90921229e0b8: extensions: add wrappedfunction() context manager
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 19578bb84731
(extensions: add unwrapfunction to undo wrapfunction, 2016-08-10).

This patch adds a simple wrappedfunction() that returns a context
manager.

Diff Detail

Repository
rHG Mercurial
Lint
Lint Skipped
Unit
Unit Tests Skipped

Event Timeline

martinvonz created this revision.Aug 22 2017, 2:29 AM
quark added subscribers: mjpieters, quark.Aug 22 2017, 5:22 PM
quark accepted this revision.

cc python expert @mjpieters to see if he has additional comments. But I think this is good enough.

mjpieters added inline comments.Aug 23 2017, 9:32 AM
mercurial/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
mjpieters requested changes to this revision.Aug 23 2017, 9:32 AM
This revision now requires changes to proceed.Aug 23 2017, 9:32 AM
quark added inline comments.Aug 23 2017, 11:44 AM
mercurial/extensions.py
408–412

The point is wrapfunction is also intended to work with non-context case:

wrapfunction(x, y, z) # works and should have immediate side effect
lothiraldan added a subscriber: lothiraldan.Aug 23 2017, 11:55 AM

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

quark added a comment.EditedAug 23 2017, 11:57 AM
In 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.

martinvonz added a comment.Aug 23 2017, 12:34 PM
In D472#7758, @quark wrote:
In 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.

mjpieters accepted this revision.Aug 24 2017, 6:14 AM

I was being too strict; like open() it's sometimes fine to apply the action in the constructor.

yuja added a subscriber: yuja.Aug 27 2017, 6:37 AM
yuja added inline comments.
mercurial/extensions.py
424

When will this __get__() be called?

wrapfunction() is generally used for wrapping a free function,
not an unbound method.

martinvonz added inline comments.Aug 27 2017, 12:41 PM
mercurial/extensions.py
424

See commit message. It's for compatibility with existing callers that bind the returned value.

yuja added inline comments.Aug 28 2017, 9:33 AM
mercurial/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.

quark added inline comments.Aug 28 2017, 1:02 PM
mercurial/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.

yuja added a comment.Aug 29 2017, 9:34 AM

This is off-topic, but if wrapfunction() is a context manager, I think
unwrapfunction() could also be a context manager.

mercurial/extensions.py
424

My point is why do we care only for classobj.method = oldfunc, which seems less likely than oldfunc().

martinvonz added a comment.Aug 29 2017, 1:00 PM
In D472#8969, @yuja wrote:

This is off-topic, but if wrapfunction() is a context manager, I think
unwrapfunction() could also be a context manager.

I agree. I think it makes sense to have wrapfunction() and unwrapfunction() and a context manager that uses both.

I also think using the return value of wrapfunction() is risky, so we should try to reduce uses of it, but let's keep that discussion out of this review.

martinvonz added a comment.Aug 29 2017, 1:01 PM

I failed with the update of this patch, so see D563 instead

quark added a comment.Aug 29 2017, 2:18 PM
In D472#9064, @martinvonz wrote:

I failed with the update of this patch, so see 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.

martinvonz edited the summary of this revision. (Show Details)Aug 29 2017, 2:20 PM
martinvonz retitled this revision from extensions: make wrapfunction() return a context manager to extensions: add wrappedfunction() context manager.
martinvonz updated this revision to Diff 1409.
martinvonz updated this revision to Diff 1413.Aug 29 2017, 8:01 PM
martinvonz added a comment.Aug 29 2017, 8:02 PM
In D472#9070, @quark wrote:
In D472#9064, @martinvonz wrote:

I failed with the update of this patch, so see 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.

Thanks, I did that and it worked.

mercurial/extensions.py
415

I fixed this to pass the wrapper to unwrapfunction() so it doesn't unwrap unrelated functions. Also updated the test.

yuja accepted this revision.Aug 30 2017, 11:21 AM

Fixed weird indent and queued, thanks.

This revision is now accepted and ready to land.Aug 30 2017, 11:21 AM
Closed by commit rHG90921229e0b8: extensions: add wrappedfunction() context manager (authored by martinvonz). · Explain WhyAug 30 2017, 11:50 AM
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

Diff 1409

mercurial/extensions.py

return wrapper(origfn, *args, **kwargs) return wrapper(origfn, *args, **kwargs)
currcls.__dict__[propname].func = wrap currcls.__dict__[propname].func = wrap
break break
if currcls is object: if currcls is object:
raise AttributeError(r"type '%s' has no property '%s'" % ( raise AttributeError(r"type '%s' has no property '%s'" % (
cls, propname)) cls, propname))
class wrappedfunction(object):
'''context manager for temporarily wrapping a function'''
def __init__(self, container, funcname, wrapper):
assert callable(wrapper)
self._container = container
self._funcname = funcname
self._wrapper = wrapper
def __enter__(self):
wrapfunction(self._container, self._funcname, self._wrapper)
mjpietersUnsubmitted
Not Done

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
mjpieters: Move this to `__enter__`. You want to be able to create the context manager separately from…
quarkUnsubmitted
Not Done

The point is wrapfunction is also intended to work with non-context case:

wrapfunction(x, y, z) # works and should have immediate side effect
quark: The point is `wrapfunction` is also intended to work with non-context case: wrapfunction(x…
def __exit__(self, exctype, excvalue, traceback):
unwrapfunction(self._container, self._funcname)
martinvonzAuthorUnsubmitted
Not Done

I fixed this to pass the wrapper to unwrapfunction() so it doesn't unwrap unrelated functions. Also updated the test.

martinvonz: I fixed this to pass the wrapper to unwrapfunction() so it doesn't unwrap unrelated functions.
def wrapfunction(container, funcname, wrapper): def wrapfunction(container, funcname, wrapper):
'''Wrap the function named funcname in container '''Wrap the function named funcname in container
Replace the funcname member in the given container with the specified Replace the funcname member in the given container with the specified
wrapper. The container is typically a module, class, or instance. wrapper. The container is typically a module, class, or instance.
The wrapper will be called like The wrapper will be called like
yujaUnsubmitted
Not Done

When will this __get__() be called?

wrapfunction() is generally used for wrapping a free function,
not an unbound method.

yuja: When will this `__get__()` be called? `wrapfunction()` is generally used for wrapping a free…
martinvonzAuthorUnsubmitted
Not Done

See commit message. It's for compatibility with existing callers that bind the returned value.

martinvonz: See commit message. It's for compatibility with existing callers that bind the returned value.
yujaUnsubmitted
Not Done

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.

yuja: I see, but I don't think it's more common than wrapping a plain function. If the compatibility…
quarkUnsubmitted
Not Done

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.

quark: I think using the return value of wrapfunction could lead to hidden bugs: oldfunc =…
yujaUnsubmitted
Not Done

My point is why do we care only for classobj.method = oldfunc, which seems less likely than oldfunc().

yuja: My point is why do we care only for `classobj.method = oldfunc`, which seems less likely than…
wrapper(orig, *args, **kwargs) wrapper(orig, *args, **kwargs)
where orig is the original (wrapped) function, and *args, **kwargs where orig is the original (wrapped) function, and *args, **kwargs
are the arguments passed to it. are the arguments passed to it.
Wrapping methods of the repository object is not recommended since Wrapping methods of the repository object is not recommended since
it conflicts with extensions that extend the repository by it conflicts with extensions that extend the repository by
subclassing. All extensions that need to extend methods of subclassing. All extensions that need to extend methods of

tests/test-extensions-wrapfunction.py

msg = str(dummy.getstack()) msg = str(dummy.getstack())
except (ValueError, IndexError) as e: except (ValueError, IndexError) as e:
msg = e.__class__.__name__ msg = e.__class__.__name__
print('unwrap %s: %s: %s' % (getid(w), getid(result), msg)) print('unwrap %s: %s: %s' % (getid(w), getid(result), msg))
batchwrap(wrappers + [wrappers[0]]) batchwrap(wrappers + [wrappers[0]])
batchunwrap([(wrappers[i] if i >= 0 else None) batchunwrap([(wrappers[i] if i >= 0 else None)
for i in [3, None, 0, 4, 0, 2, 1, None]]) for i in [3, None, 0, 4, 0, 2, 1, None]])
wrap0 = extensions.wrappedfunction(dummy, 'getstack', wrappers[0])
wrap1 = extensions.wrappedfunction(dummy, 'getstack', wrappers[1])
# Use them in a different order from how they were created to check that
# the wrapping happens in __enter__, not in __init__
print('context manager', dummy.getstack())
with wrap1:
print('context manager', dummy.getstack())
with wrap0:
print('context manager', dummy.getstack())
print('context manager', dummy.getstack())
print('context manager', dummy.getstack())

tests/test-extensions-wrapfunction.py.out

wrap 0: [0, 'orig'] wrap 0: [0, 'orig']
wrap 1: [1, 0, 'orig'] wrap 1: [1, 0, 'orig']
wrap 2: [2, 1, 0, 'orig'] wrap 2: [2, 1, 0, 'orig']
wrap 3: [3, 2, 1, 0, 'orig'] wrap 3: [3, 2, 1, 0, 'orig']
wrap 4: [4, 3, 2, 1, 0, 'orig'] wrap 4: [4, 3, 2, 1, 0, 'orig']
wrap 0: [0, 4, 3, 2, 1, 0, 'orig'] wrap 0: [0, 4, 3, 2, 1, 0, 'orig']
unwrap 3: 3: [0, 4, 2, 1, 0, 'orig'] unwrap 3: 3: [0, 4, 2, 1, 0, 'orig']
unwrap -: 0: [4, 2, 1, 0, 'orig'] unwrap -: 0: [4, 2, 1, 0, 'orig']
unwrap 0: 0: [4, 2, 1, 'orig'] unwrap 0: 0: [4, 2, 1, 'orig']
unwrap 4: 4: [2, 1, 'orig'] unwrap 4: 4: [2, 1, 'orig']
unwrap 0: -: ValueError unwrap 0: -: ValueError
unwrap 2: 2: [1, 'orig'] unwrap 2: 2: [1, 'orig']
unwrap 1: 1: ['orig'] unwrap 1: 1: ['orig']
unwrap -: -: IndexError unwrap -: -: IndexError
context manager ['orig']
context manager [1, 'orig']
context manager [0, 1, 'orig']
context manager [1, 'orig']
context manager ['orig']