Submitter | Gregory Szorc |
---|---|
Date | Aug. 19, 2014, 5:30 a.m. |
Message ID | <110ff56e99e2ccff0384.1408426213@vm-ubuntu-main.gateway.sonic.net> |
Download | mbox | patch |
Permalink | /patch/5502/ |
State | Changes Requested |
Headers | show |
Comments
On 8/18/2014 10:30 PM, Gregory Szorc wrote: > # HG changeset patch > # User Gregory Szorc <gregory.szorc@gmail.com> > # Date 1408422497 25200 > # Mon Aug 18 21:28:17 2014 -0700 > # Node ID 110ff56e99e2ccff03841cc8baeed7f14c35f69b > # Parent 8dda6f6ff564d8fe6ac7b8ce4c74eb9bfb5de14a > util: add event handling mechanism > diff --git a/tests/test-eventmanager.py b/tests/test-eventmanager.py > new file mode 100644 > --- /dev/null > +++ b/tests/test-eventmanager.py > @@ -0,0 +1,109 @@ > +from mercurial.util import event, eventmanager, safehasattr > + > +import unittest > +import silenttestrunner > + > +class testeventmanager(unittest.TestCase): > + def test_eventsimple(self): test-check-commit-hg.t doesn't like this commit because it is introducing a function with underscores. But I just stole this naming convention from other unittest-based .py tests. I'm not sure who is in the wrong here. Dropping the _ still runs the tests.
On Mon, Aug 18, 2014 at 10:53:21PM -0700, Gregory Szorc wrote: > On 8/18/2014 10:30 PM, Gregory Szorc wrote: > > # HG changeset patch > > # User Gregory Szorc <gregory.szorc@gmail.com> > > # Date 1408422497 25200 > > # Mon Aug 18 21:28:17 2014 -0700 > > # Node ID 110ff56e99e2ccff03841cc8baeed7f14c35f69b > > # Parent 8dda6f6ff564d8fe6ac7b8ce4c74eb9bfb5de14a > > util: add event handling mechanism > > > diff --git a/tests/test-eventmanager.py b/tests/test-eventmanager.py > > new file mode 100644 > > --- /dev/null > > +++ b/tests/test-eventmanager.py > > @@ -0,0 +1,109 @@ > > +from mercurial.util import event, eventmanager, safehasattr > > + > > +import unittest > > +import silenttestrunner > > + > > +class testeventmanager(unittest.TestCase): > > + def test_eventsimple(self): > > test-check-commit-hg.t doesn't like this commit because it is > introducing a function with underscores. But I just stole this naming > convention from other unittest-based .py tests. I'm not sure who is in > the wrong here. > > Dropping the _ still runs the tests. I'm going to claim that test-check-commit-hg.t is too picky here, and that consistency with the other unittest-derived testcases is worthwhile. A fix doesn't seem immediately obvious, but I'm also happy to just ignore it for now. > _______________________________________________ > Mercurial-devel mailing list > Mercurial-devel@selenic.com > http://selenic.com/mailman/listinfo/mercurial-devel
On Mon, Aug 18, 2014 at 10:30:13PM -0700, Gregory Szorc wrote: > # HG changeset patch > # User Gregory Szorc <gregory.szorc@gmail.com> > # Date 1408422497 25200 > # Mon Aug 18 21:28:17 2014 -0700 > # Node ID 110ff56e99e2ccff03841cc8baeed7f14c35f69b > # Parent 8dda6f6ff564d8fe6ac7b8ce4c74eb9bfb5de14a > util: add event handling mechanism > > The patch adds a generic event handling mechanism to Mercurial. From a > high level, you create a named event, register some functions with that > event, and when you fire that event the registered functions get called. Interesting. I think this is probably worth pursuing in some form, but I suspect mpm will take some amount of convincing. > > As will be demonstrated in subsequent patches, event handling can be > considered an internal hooks mechanism and will provide a better > alternative to wrapping or monkeypatching. > > The intent of the events system is to give extensions more well-defined > points for code insertion. Currently, extension authors have a limited > set of hooks and a giant pile of functions to choose from. When hooks > won't satisfy your requirements, you need to dig through a pile of code > to find an appropriate function to intercept. Then you need to replace > it and hope you remembered to call the original function properly. > It's a dangerous, inexact science. > > Events, by contrast, will provide a well-defined set of points for > extensions to inject code. These require a simple code grep to locate > (".events.register()") and are less prone to common mistakes, such as > calling the original function incorrectly. Calling out some example use cases here might go a bit towards pitching it (that is, right in the commit message). I suspect you have some bugzilla or codereview reasons to want this, and I probably want them for some reason as well. > > Events have another advantage in that they can be instance specific. > Monkeypatching often results in changing symbols on modules or > class types as opposed to individual methods on individual object > instances. Oftentimes you only want to apply customization to a single > instance of an object if that object meets certain criteria. In the > current world, you often have to globally replace and filter out > invocations that aren't appropriate. This is very dangerous. Events > support modifying behavior at a per-instance level and are thus much > safer. > > diff --git a/mercurial/util.py b/mercurial/util.py > --- a/mercurial/util.py > +++ b/mercurial/util.py > @@ -2039,8 +2039,94 @@ class hooks(object): > for source, hook in self._hooks: > results.append(hook(*args)) > return results > > +class event(object): > + '''An event with its handlers. > + > + An ``event`` is essentially a collection of functions that will be invoked > + when the event fires. ``event`` instances are typically created through > + ``eventmanager`` instances. > + > + Handler functions can be registered against an instance via the ``+=`` > + operator. They can be unregistered via the ``-=`` operator. Cute, but not very obvious. Can you make it .register/.(unregister|clear) instead of doing the operator overload? > + > + Handler functions can be invoked by calling an ``event`` instance like > + it is a function. > + > + Handlers are executed in the order they are registered. Does that actually matter? Might not be worth specifying in the API contract like this. > + > + The return value of handler functions is ignored. > + > + When events are created, they are "bound" to 0 or more values which will > + be passed to every handler function in addition to the values passed to > + that event. > + > + e.g. > + > + >>> def handler(x, y, z): > + ... print '%s %s %s' % (x, y, z) > + >>> e = event('foo', 'bar') > + >>> e += handler > + >>> e('baz') > + foo bar baz > + ''' > + > + def __init__(self, *args): > + # Convert to list to facilitate + operator later. > + self._args = list(args) > + self._handlers = [] > + > + def __iadd__(self, fn): > + if fn not in self._handlers: > + self._handlers.append(fn) > + return self > + > + def __isub__(self, fn): > + self._handlers.remove(fn) > + return self > + > + def __len__(self): > + return len(self._handlers) > + > + def __call__(self, *args, **kwargs): > + args = self._args + list(args) > + for fn in self._handlers: > + fn(*args, **kwargs) > + > +class eventmanager(object): > + '''A collection of events. > + > + This class powers the internal events system. Instances of this class are > + typically attached to an object, but they can be standalone. > + > + Events are registered by calling the ``register`` function. Afterwards, > + the created ``event`` instance will be available under a property of the > + registered name. > + > + e.g. > + > + >>> m = eventmanager() > + >>> m.register('oncreate') > + >>> def handler(): > + ... print 'hello' > + >>> m.oncreate += handler > + >>> m.oncreate() > + hello > + ''' > + def register(self, name, *args): > + """Register an event. > + > + Events have names and default positional arguments. If positional > + arguments are defined, these argument will be passed as the first > + arguments to every invocation of the event. > + > + A common pattern is to pass the ``self`` handle on whatever object > + is holding this ``eventmanager`` instance. > + """ > + if not safehasattr(self, name): > + setattr(self, name, event(*args)) > + > def debugstacktrace(msg='stacktrace', skip=0, f=sys.stderr, otherf=sys.stdout): > '''Writes a message to f (stderr) with a nicely formatted stacktrace. > Skips the 'skip' last entries. By default it will flush stdout first. > It can be used everywhere and do intentionally not require an ui object. > diff --git a/tests/test-eventmanager.py b/tests/test-eventmanager.py > new file mode 100644 > --- /dev/null > +++ b/tests/test-eventmanager.py > @@ -0,0 +1,109 @@ > +from mercurial.util import event, eventmanager, safehasattr > + > +import unittest > +import silenttestrunner > + > +class testeventmanager(unittest.TestCase): > + def test_eventsimple(self): > + e = event() > + self.assertEqual(len(e), 0) > + > + calls = {'h1': 0, 'h2': 0} > + > + def h1(): > + calls['h1'] += 1 > + > + def h2(): > + calls['h2'] += 1 > + > + e += h1 > + self.assertEqual(len(e), 1) > + e += h2 > + self.assertEqual(len(e), 2) > + e += h2 > + self.assertEqual(len(e), 2) > + > + e() > + self.assertEqual(calls, {'h1': 1, 'h2': 1}) > + e() > + self.assertEqual(calls, {'h1': 2, 'h2': 2}) > + > + e -= h1 > + e() > + self.assertEqual(calls, {'h1': 2, 'h2': 3}) > + > + def test_eventarguments(self): > + e = event() > + > + calls = {'h1': [], 'h2': []} > + > + def h1(foo, bar, baz=False): > + calls['h1'].append((foo, bar, baz)) > + > + def h2(foo, bar, baz=True): > + calls['h2'].append((foo, bar, baz)) > + > + e += h1 > + e += h2 > + > + e(1, 2, 3) > + self.assertEqual(calls, {'h1': [(1, 2, 3)], 'h2': [(1, 2, 3)]}) > + e(3, 4, baz=None) > + self.assertEqual(calls, {'h1': [(1, 2, 3), (3, 4, None)], > + 'h2': [(1, 2, 3), (3, 4, None)]}) > + e(5, 6) > + self.assertEqual(calls, {'h1': [(1, 2, 3), (3, 4, None), (5, 6, False)], > + 'h2': [(1, 2, 3), (3, 4, None), (5, 6, True)]}) > + > + def test_defaultargs(self): > + expected = [1, True] > + def h(obj, foo, bar=True): > + self.assertEqual(o, obj) > + self.assertEqual(foo, expected[0]) > + self.assertEqual(bar, expected[1]) > + > + o = object() > + e = event(o) > + e += h > + > + e(1) > + expected = [2, False] > + e(2, bar=False) > + > + def test_eventmanager(self): > + class eventobject(object): > + def __init__(self): > + self.events = eventmanager() > + > + o = eventobject() > + self.assertFalse(safehasattr(o.events, 'e1')) > + try: > + o.events.e1() > + except AttributeError: > + pass > + o.events.register('e1') > + self.assertTrue(safehasattr(o.events, 'e1')) > + > + obj = object() > + o.events.register('e2', obj) > + self.assertTrue(safehasattr(o.events, 'e2')) > + > + calls = {'e1': 0, 'e2': []} > + > + def e1h(): > + calls['e1'] += 1 > + > + o.events.e1 += e1h > + o.events.e1() > + self.assertEqual(calls, {'e1': 1, 'e2': []}) > + > + def e2h(obj2, foo, bar): > + self.assertEqual(obj2, obj) > + calls['e2'].append((foo, bar)) > + > + o.events.e2 += e2h > + o.events.e2('1', '2') > + self.assertEqual(calls, {'e1': 1, 'e2': [('1', '2')]}) > + > +if __name__ == '__main__': > + silenttestrunner.main(__name__) > _______________________________________________ > Mercurial-devel mailing list > Mercurial-devel@selenic.com > http://selenic.com/mailman/listinfo/mercurial-devel
On Mon, 2014-08-18 at 22:53 -0700, Gregory Szorc wrote: > On 8/18/2014 10:30 PM, Gregory Szorc wrote: > > # HG changeset patch > > # User Gregory Szorc <gregory.szorc@gmail.com> > > # Date 1408422497 25200 > > # Mon Aug 18 21:28:17 2014 -0700 > > # Node ID 110ff56e99e2ccff03841cc8baeed7f14c35f69b > > # Parent 8dda6f6ff564d8fe6ac7b8ce4c74eb9bfb5de14a > > util: add event handling mechanism > > > diff --git a/tests/test-eventmanager.py b/tests/test-eventmanager.py > > new file mode 100644 > > --- /dev/null > > +++ b/tests/test-eventmanager.py > > @@ -0,0 +1,109 @@ > > +from mercurial.util import event, eventmanager, safehasattr > > + > > +import unittest > > +import silenttestrunner > > + > > +class testeventmanager(unittest.TestCase): > > + def test_eventsimple(self): > > test-check-commit-hg.t doesn't like this commit because it is > introducing a function with underscores. But I just stole this naming > convention from other unittest-based .py tests. I'm not sure who is in > the wrong here. The unittest framework is wrong, as is most of the Python standard library: just compare it to the core language. More seriously, we DO need to enforce this somewhere, we can't enforce it in check-code yet, and check-commit isn't remotely sophisticated enough yet to handle exceptional cases.
On 8/19/14 11:22 PM, Matt Mackall wrote: > On Mon, 2014-08-18 at 22:53 -0700, Gregory Szorc wrote: >> On 8/18/2014 10:30 PM, Gregory Szorc wrote: >>> # HG changeset patch >>> # User Gregory Szorc <gregory.szorc@gmail.com> >>> # Date 1408422497 25200 >>> # Mon Aug 18 21:28:17 2014 -0700 >>> # Node ID 110ff56e99e2ccff03841cc8baeed7f14c35f69b >>> # Parent 8dda6f6ff564d8fe6ac7b8ce4c74eb9bfb5de14a >>> util: add event handling mechanism >> >>> diff --git a/tests/test-eventmanager.py b/tests/test-eventmanager.py >>> new file mode 100644 >>> --- /dev/null >>> +++ b/tests/test-eventmanager.py >>> @@ -0,0 +1,109 @@ >>> +from mercurial.util import event, eventmanager, safehasattr >>> + >>> +import unittest >>> +import silenttestrunner >>> + >>> +class testeventmanager(unittest.TestCase): >>> + def test_eventsimple(self): >> >> test-check-commit-hg.t doesn't like this commit because it is >> introducing a function with underscores. But I just stole this naming >> convention from other unittest-based .py tests. I'm not sure who is in >> the wrong here. > > The unittest framework is wrong, as is most of the Python standard > library: just compare it to the core language. PEP-8 says to use functions_with_underscores not functionsasoneword. unittest is a bit inconsistent: unittest.TestCase has both camelCase and lowercase methods. I'm not sure whether this is historical or an attempt to distinguish and/or avoid conflicts between unittest "core" APIs and user/test-supplied code. Viewed through PEP-8, Mercurial's style convention is what's "wrong." Anyway, the default regexp in unittest for discovering test methods allows testwithoutunderscore. So if you tell me to remove it, I will. mpm: I'd appreciate your feedback on the event part of this proposal. My extensions that override push behavior really need something like this. See e.g. the 2 functions starting at https://hg.mozilla.org/hgcustom/version-control-tools/file/d50b97f6e22f/hgext/reviewboard/client.py#l128
On Wed, 2014-08-20 at 09:16 -0700, Gregory Szorc wrote: > On 8/19/14 11:22 PM, Matt Mackall wrote: > > On Mon, 2014-08-18 at 22:53 -0700, Gregory Szorc wrote: > >> On 8/18/2014 10:30 PM, Gregory Szorc wrote: > >>> # HG changeset patch > >>> # User Gregory Szorc <gregory.szorc@gmail.com> > >>> # Date 1408422497 25200 > >>> # Mon Aug 18 21:28:17 2014 -0700 > >>> # Node ID 110ff56e99e2ccff03841cc8baeed7f14c35f69b > >>> # Parent 8dda6f6ff564d8fe6ac7b8ce4c74eb9bfb5de14a > >>> util: add event handling mechanism > >> > >>> diff --git a/tests/test-eventmanager.py b/tests/test-eventmanager.py > >>> new file mode 100644 > >>> --- /dev/null > >>> +++ b/tests/test-eventmanager.py > >>> @@ -0,0 +1,109 @@ > >>> +from mercurial.util import event, eventmanager, safehasattr > >>> + > >>> +import unittest > >>> +import silenttestrunner > >>> + > >>> +class testeventmanager(unittest.TestCase): > >>> + def test_eventsimple(self): > >> > >> test-check-commit-hg.t doesn't like this commit because it is > >> introducing a function with underscores. But I just stole this naming > >> convention from other unittest-based .py tests. I'm not sure who is in > >> the wrong here. > > > > The unittest framework is wrong, as is most of the Python standard > > library: just compare it to the core language. > > PEP-8 says to use functions_with_underscores not functionsasoneword. > unittest is a bit inconsistent: unittest.TestCase has both camelCase and > lowercase methods. I'm not sure whether this is historical or an attempt > to distinguish and/or avoid conflicts between unittest "core" APIs and > user/test-supplied code. > > Viewed through PEP-8, Mercurial's style convention is what's "wrong." Please do "pydoc list|dict|file|str|etc" and count the separators. Also look at dir(__builtins__). It's clear that Python's core style is no separator. About the only common exception to the rule in the core language is dict.has_key.. and that got removed in Py3. The actual PEP8 recommendation is: "Function names should be lowercase, with words separated by underscores as necessary to improve readability." The second clause has that vague "as necessary" which in practice means "when random developer feels like it". This was a mistake, because they ended up sacrificing a huge amount of consistency (aka writability) for a pretty small amount of readability. And it even allows monstrosities like "set_debuglevel" because it doesn't even suggest you should be consistent inside a single identifier! They should have chose one way or the other, always... and the obvious choice would have been _to match the language itself_. So I recommend reading that "as necessary" as "never".
Patch
diff --git a/mercurial/util.py b/mercurial/util.py --- a/mercurial/util.py +++ b/mercurial/util.py @@ -2039,8 +2039,94 @@ class hooks(object): for source, hook in self._hooks: results.append(hook(*args)) return results +class event(object): + '''An event with its handlers. + + An ``event`` is essentially a collection of functions that will be invoked + when the event fires. ``event`` instances are typically created through + ``eventmanager`` instances. + + Handler functions can be registered against an instance via the ``+=`` + operator. They can be unregistered via the ``-=`` operator. + + Handler functions can be invoked by calling an ``event`` instance like + it is a function. + + Handlers are executed in the order they are registered. + + The return value of handler functions is ignored. + + When events are created, they are "bound" to 0 or more values which will + be passed to every handler function in addition to the values passed to + that event. + + e.g. + + >>> def handler(x, y, z): + ... print '%s %s %s' % (x, y, z) + >>> e = event('foo', 'bar') + >>> e += handler + >>> e('baz') + foo bar baz + ''' + + def __init__(self, *args): + # Convert to list to facilitate + operator later. + self._args = list(args) + self._handlers = [] + + def __iadd__(self, fn): + if fn not in self._handlers: + self._handlers.append(fn) + return self + + def __isub__(self, fn): + self._handlers.remove(fn) + return self + + def __len__(self): + return len(self._handlers) + + def __call__(self, *args, **kwargs): + args = self._args + list(args) + for fn in self._handlers: + fn(*args, **kwargs) + +class eventmanager(object): + '''A collection of events. + + This class powers the internal events system. Instances of this class are + typically attached to an object, but they can be standalone. + + Events are registered by calling the ``register`` function. Afterwards, + the created ``event`` instance will be available under a property of the + registered name. + + e.g. + + >>> m = eventmanager() + >>> m.register('oncreate') + >>> def handler(): + ... print 'hello' + >>> m.oncreate += handler + >>> m.oncreate() + hello + ''' + def register(self, name, *args): + """Register an event. + + Events have names and default positional arguments. If positional + arguments are defined, these argument will be passed as the first + arguments to every invocation of the event. + + A common pattern is to pass the ``self`` handle on whatever object + is holding this ``eventmanager`` instance. + """ + if not safehasattr(self, name): + setattr(self, name, event(*args)) + def debugstacktrace(msg='stacktrace', skip=0, f=sys.stderr, otherf=sys.stdout): '''Writes a message to f (stderr) with a nicely formatted stacktrace. Skips the 'skip' last entries. By default it will flush stdout first. It can be used everywhere and do intentionally not require an ui object. diff --git a/tests/test-eventmanager.py b/tests/test-eventmanager.py new file mode 100644 --- /dev/null +++ b/tests/test-eventmanager.py @@ -0,0 +1,109 @@ +from mercurial.util import event, eventmanager, safehasattr + +import unittest +import silenttestrunner + +class testeventmanager(unittest.TestCase): + def test_eventsimple(self): + e = event() + self.assertEqual(len(e), 0) + + calls = {'h1': 0, 'h2': 0} + + def h1(): + calls['h1'] += 1 + + def h2(): + calls['h2'] += 1 + + e += h1 + self.assertEqual(len(e), 1) + e += h2 + self.assertEqual(len(e), 2) + e += h2 + self.assertEqual(len(e), 2) + + e() + self.assertEqual(calls, {'h1': 1, 'h2': 1}) + e() + self.assertEqual(calls, {'h1': 2, 'h2': 2}) + + e -= h1 + e() + self.assertEqual(calls, {'h1': 2, 'h2': 3}) + + def test_eventarguments(self): + e = event() + + calls = {'h1': [], 'h2': []} + + def h1(foo, bar, baz=False): + calls['h1'].append((foo, bar, baz)) + + def h2(foo, bar, baz=True): + calls['h2'].append((foo, bar, baz)) + + e += h1 + e += h2 + + e(1, 2, 3) + self.assertEqual(calls, {'h1': [(1, 2, 3)], 'h2': [(1, 2, 3)]}) + e(3, 4, baz=None) + self.assertEqual(calls, {'h1': [(1, 2, 3), (3, 4, None)], + 'h2': [(1, 2, 3), (3, 4, None)]}) + e(5, 6) + self.assertEqual(calls, {'h1': [(1, 2, 3), (3, 4, None), (5, 6, False)], + 'h2': [(1, 2, 3), (3, 4, None), (5, 6, True)]}) + + def test_defaultargs(self): + expected = [1, True] + def h(obj, foo, bar=True): + self.assertEqual(o, obj) + self.assertEqual(foo, expected[0]) + self.assertEqual(bar, expected[1]) + + o = object() + e = event(o) + e += h + + e(1) + expected = [2, False] + e(2, bar=False) + + def test_eventmanager(self): + class eventobject(object): + def __init__(self): + self.events = eventmanager() + + o = eventobject() + self.assertFalse(safehasattr(o.events, 'e1')) + try: + o.events.e1() + except AttributeError: + pass + o.events.register('e1') + self.assertTrue(safehasattr(o.events, 'e1')) + + obj = object() + o.events.register('e2', obj) + self.assertTrue(safehasattr(o.events, 'e2')) + + calls = {'e1': 0, 'e2': []} + + def e1h(): + calls['e1'] += 1 + + o.events.e1 += e1h + o.events.e1() + self.assertEqual(calls, {'e1': 1, 'e2': []}) + + def e2h(obj2, foo, bar): + self.assertEqual(obj2, obj) + calls['e2'].append((foo, bar)) + + o.events.e2 += e2h + o.events.e2('1', '2') + self.assertEqual(calls, {'e1': 1, 'e2': [('1', '2')]}) + +if __name__ == '__main__': + silenttestrunner.main(__name__)