> On Dec. 4, 2014, 5:21 p.m., Maxim Khutornenko wrote: > > src/main/python/apache/aurora/client/cli/__init__.py, lines 172-175 > > <https://reviews.apache.org/r/28693/diff/1/?file=782496#file782496line172> > > > > Why not keeping @abstractmethod attributes and dropping the return > > statements instead? With your modification there is no need to keep this > > noop behavior as tests like "EmptyPlugin" below would not be possible > > anyway. > > Zameer Manji wrote: > The return statements provide default behaviour that plugins should have > to prevent AURORA-362. If get_options does not return an iterable the > argument handling code will crash and before_dispatch needs to return > raw_args to prevent the clobbering of user passed in commandline arguments. > > This reduces the work that plugins need to do if they are just interested > in implementing before_execution or after_execution.
I don't see how `before_dispatch` is semantically different from `before_execution` for the plugin implementor. The only difference is that one is expected to return `raw_args` and the other one is void. These noops provide no additional value besides questionable convenience on override. I would say drop the return statements, convert these methods back to abstract and properly document return type expectations. This way there is no second guessing what needs to be overriden and what isn't. - Maxim ----------------------------------------------------------- This is an automatically generated e-mail. To reply, visit: https://reviews.apache.org/r/28693/#review63859 ----------------------------------------------------------- On Dec. 4, 2014, 6:36 a.m., Zameer Manji wrote: > > ----------------------------------------------------------- > This is an automatically generated e-mail. To reply, visit: > https://reviews.apache.org/r/28693/ > ----------------------------------------------------------- > > (Updated Dec. 4, 2014, 6:36 a.m.) > > > Review request for Aurora, Kevin Sweeney and Maxim Khutornenko. > > > Repository: aurora > > > Description > ------- > > This makes ConfigurationPlugin inherit from AbstractClass so the > @abstractmethod annotation is useful. This also removes the annotation for > the two methods with default values. > > > Diffs > ----- > > src/main/python/apache/aurora/client/cli/__init__.py > 6e553d8af459e575b2d62282a3bc0d1e266203d8 > src/test/python/apache/aurora/client/cli/test_plugins.py > 7a0a31818cbc57de952d7817f8e7c8fa1e84b25a > > Diff: https://reviews.apache.org/r/28693/diff/ > > > Testing > ------- > > ./pants src/test/python/apache/aurora/client:: > > > Thanks, > > Zameer Manji > >
