Chromium Code Reviews
|
|
Chromium Code Reviews
Issue
1171943002:
[test] Refactoring - Use subject/observer pattern for progress indicators. (Closed)
|
Created:
5 years, 6 months ago by Michael Achenbach Modified:
5 years, 6 months ago CC:
v8-dev Base URL:
https://chromium.googlesource.com/v8/v8.git@master Target Ref:
refs/pending/heads/master Project:
v8 Visibility:
Public. |
Description[test] Refactoring - Use subject/observer pattern for progress indicators.
This should prevent bugs caused by missing super calls in
overridden methods. The assumption is that methods of
different indicators are independent.
Committed: https://crrev.com/fbe973ff1722a6158a5b2babce9c1a32d26a1d3b
Cr-Commit-Position: refs/heads/master@{#28866}
Patch Set 1 #
Total comments: 13
Patch Set 2 : Review #
Total comments: 2
Patch Set 3 : Review #
Messages
Total messages: 26 (9 generated)
The CQ bit was checked by machenbach@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1171943002/1
machenbach@chromium.org changed reviewers: + jkummerow@chromium.org, tandrii@chromium.org
PTAL
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/1171943002/diff/1/tools/testrunner/local/prog... File tools/testrunner/local/progress.py (right): https://codereview.chromium.org/1171943002/diff/1/tools/testrunner/local/prog... tools/testrunner/local/progress.py:52: class IndicatorNotifier: inherit from "(object)" https://codereview.chromium.org/1171943002/diff/1/tools/testrunner/local/prog... tools/testrunner/local/progress.py:57: def register(self, indicator): nit: maybe Register for consistency with code around? https://codereview.chromium.org/1171943002/diff/1/tools/testrunner/local/prog... tools/testrunner/local/progress.py:61: """Generic event.""" how about: """Generic event dispatcher.""" ? and I'd add an assert: assert callable(ProgressIndicator.__dict__[func_name]), "unknown event: %s" % func_name https://codereview.chromium.org/1171943002/diff/1/tools/testrunner/local/prog... tools/testrunner/local/progress.py:64: getattr(indicator, func_name)(*args, **kwargs) maybe also add setattr(self, func_name, func) ? https://codereview.chromium.org/1171943002/diff/1/tools/testrunner/local/prog... tools/testrunner/local/progress.py:70: def SetRunner(self, runner): IMO, I'd keep the constructor. Am I right that SetRunner is really for documentation, not really for anything else? because bla.runner = runner still works anyways...
Looks good. https://codereview.chromium.org/1171943002/diff/1/tools/testrunner/local/prog... File tools/testrunner/local/progress.py (right): https://codereview.chromium.org/1171943002/diff/1/tools/testrunner/local/prog... tools/testrunner/local/progress.py:70: def SetRunner(self, runner): On 2015/06/09 09:18:01, tandrii(chromium) wrote: > IMO, I'd keep the constructor. Agreed. The constructor exists mostly for documentation purposes too -- the idea is that looking at the constructor is enough to understand the object layout, as all fields are initialized there. > Am I right that SetRunner is really for documentation, not really for anything > else? because bla.runner = runner still works anyways... I guess unless setattr is implemented as you suggested above, exposing a setter is the only [elegant] way to set the runner for all registered progress indicators.
https://codereview.chromium.org/1171943002/diff/1/tools/testrunner/local/prog... File tools/testrunner/local/progress.py (right): https://codereview.chromium.org/1171943002/diff/1/tools/testrunner/local/prog... tools/testrunner/local/progress.py:70: def SetRunner(self, runner): On 2015/06/09 09:49:51, Jakob wrote: > On 2015/06/09 09:18:01, tandrii(chromium) wrote: > > IMO, I'd keep the constructor. > > Agreed. The constructor exists mostly for documentation purposes too -- the idea > is that looking at the constructor is enough to understand the object layout, as > all fields are initialized there. > > > Am I right that SetRunner is really for documentation, not really for anything > > else? because bla.runner = runner still works anyways... > > I guess unless setattr is implemented as you suggested above, exposing a setter > is the only [elegant] way to set the runner for all registered progress > indicators. I don't think setattr above will change anything, as it only binds functions. Setters are not pythonic, properties are. Yet i'm guilty of using them myself, purely for being more explicit = e.g., documentation. So, don't mind having it SetRunner() here :)
Thanks to Andrii's help, the code is now statically initializing the generic methods. Now there shouldn't be any perf impact. https://codereview.chromium.org/1171943002/diff/1/tools/testrunner/local/prog... File tools/testrunner/local/progress.py (right): https://codereview.chromium.org/1171943002/diff/1/tools/testrunner/local/prog... tools/testrunner/local/progress.py:52: class IndicatorNotifier: On 2015/06/09 09:18:01, tandrii(chromium) wrote: > inherit from "(object)" Done. https://codereview.chromium.org/1171943002/diff/1/tools/testrunner/local/prog... tools/testrunner/local/progress.py:57: def register(self, indicator): On 2015/06/09 09:18:01, tandrii(chromium) wrote: > nit: maybe Register for consistency with code around? Done. https://codereview.chromium.org/1171943002/diff/1/tools/testrunner/local/prog... tools/testrunner/local/progress.py:61: """Generic event.""" On 2015/06/09 09:18:01, tandrii(chromium) wrote: > how about: """Generic event dispatcher.""" ? > > and I'd add an assert: > > assert callable(ProgressIndicator.__dict__[func_name]), "unknown event: %s" % > func_name now changed design... https://codereview.chromium.org/1171943002/diff/1/tools/testrunner/local/prog... tools/testrunner/local/progress.py:64: getattr(indicator, func_name)(*args, **kwargs) On 2015/06/09 09:18:01, tandrii(chromium) wrote: > maybe also add setattr(self, func_name, func) ? now changed design... https://codereview.chromium.org/1171943002/diff/1/tools/testrunner/local/prog... tools/testrunner/local/progress.py:70: def SetRunner(self, runner): On 2015/06/09 10:14:34, tandrii(chromium) wrote: > On 2015/06/09 09:49:51, Jakob wrote: > > On 2015/06/09 09:18:01, tandrii(chromium) wrote: > > > IMO, I'd keep the constructor. > > > > Agreed. The constructor exists mostly for documentation purposes too -- the > idea > > is that looking at the constructor is enough to understand the object layout, > as > > all fields are initialized there. > > > > > Am I right that SetRunner is really for documentation, not really for > anything > > > else? because bla.runner = runner still works anyways... > > > > I guess unless setattr is implemented as you suggested above, exposing a > setter > > is the only [elegant] way to set the runner for all registered progress > > indicators. > > I don't think setattr above will change anything, as it only binds functions. > Setters are not pythonic, properties are. Yet i'm guilty of using them myself, > purely for being more explicit = e.g., documentation. So, don't mind having it > SetRunner() here :) I keep the constructor, but I also need the setter to not be required to make also attribute assignment generic in the IndicatorNotifier (if I add a generic __setattr__ it also collides with the initialization of self.indicators = []).
The CQ bit was checked by machenbach@chromium.org to run a CQ dry run
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1171943002/20001
lgtm https://codereview.chromium.org/1171943002/diff/1/tools/testrunner/local/prog... File tools/testrunner/local/progress.py (right): https://codereview.chromium.org/1171943002/diff/1/tools/testrunner/local/prog... tools/testrunner/local/progress.py:70: def SetRunner(self, runner): On 2015/06/09 12:47:14, Michael Achenbach wrote: > On 2015/06/09 10:14:34, tandrii(chromium) wrote: > > On 2015/06/09 09:49:51, Jakob wrote: > > > On 2015/06/09 09:18:01, tandrii(chromium) wrote: > > > > IMO, I'd keep the constructor. > > > > > > Agreed. The constructor exists mostly for documentation purposes too -- the > > idea > > > is that looking at the constructor is enough to understand the object > layout, > > as > > > all fields are initialized there. > > > > > > > Am I right that SetRunner is really for documentation, not really for > > anything > > > > else? because bla.runner = runner still works anyways... > > > > > > I guess unless setattr is implemented as you suggested above, exposing a > > setter > > > is the only [elegant] way to set the runner for all registered progress > > > indicators. > > > > I don't think setattr above will change anything, as it only binds functions. > > Setters are not pythonic, properties are. Yet i'm guilty of using them myself, > > purely for being more explicit = e.g., documentation. So, don't mind having it > > SetRunner() here :) > > I keep the constructor, but I also need the setter to not be required to make > also attribute assignment generic in the IndicatorNotifier (if I add a generic > __setattr__ it also collides with the initialization of self.indicators = []). Acknowledged. https://codereview.chromium.org/1171943002/diff/20001/tools/testrunner/local/... File tools/testrunner/local/progress.py (right): https://codereview.chromium.org/1171943002/diff/20001/tools/testrunner/local/... tools/testrunner/local/progress.py:96: def _Indicator_apply(func): nit: then maybe _IndicatorNotifier_apply, and even better move its body inside the loop, but make sure to keep func.__name__ instead of func_name to avoid closure by ref problem.
Funky. LGTM.
The CQ bit was unchecked by commit-bot@chromium.org
Dry run: This issue passed the CQ dry run.
https://codereview.chromium.org/1171943002/diff/20001/tools/testrunner/local/... File tools/testrunner/local/progress.py (right): https://codereview.chromium.org/1171943002/diff/20001/tools/testrunner/local/... tools/testrunner/local/progress.py:96: def _Indicator_apply(func): On 2015/06/09 13:24:45, tandrii(chromium) wrote: > nit: then maybe _IndicatorNotifier_apply, and even better move its body inside > the loop, but make sure to keep func.__name__ instead of func_name to avoid > closure by ref problem. Done. Even funkier...
The CQ bit was checked by machenbach@chromium.org to run a CQ dry run
The patchset sent to the CQ was uploaded after l-g-t-m from jkummerow@chromium.org, tandrii@chromium.org Link to the patchset: https://codereview.chromium.org/1171943002/#ps40001 (title: "Review")
Dry run: CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1171943002/40001
The CQ bit was unchecked by machenbach@chromium.org
The CQ bit was checked by machenbach@chromium.org
CQ is trying da patch. Follow status at https://chromium-cq-status.appspot.com/patch-status/1171943002/40001
Message was sent while issue was closed.
Committed patchset #3 (id:40001)
Message was sent while issue was closed.
Patchset 3 (id:??) landed as https://crrev.com/fbe973ff1722a6158a5b2babce9c1a32d26a1d3b Cr-Commit-Position: refs/heads/master@{#28866}
Message was sent while issue was closed.
A revert of this CL (patchset #3 id:40001) has been created in https://codereview.chromium.org/1163373005/ by machenbach@chromium.org. The reason for reverting is: might break stuff. |
||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||||
