Issue 6569732794744832: Issue 2200 - PART I/II - Migrate to argparse (Closed)
|
Created:
April 7, 2015, 1:22 p.m. by mathias Modified:
April 8, 2015, 11:49 a.m. CC:
Fred, René Jeschke Visibility:
Public. |
DescriptionSee https://issues.adblockplus.org/ticket/2200 for more information..
Patch Set 1 #
Total comments: 14
Patch Set 2 : Issue 2200 - PART I/II - Migrate to argparse #
Total comments: 4
Patch Set 3 : Issue 2200 - PART I/II - Migrate to argparse #MessagesTotal messages: 8
http://codereview.adblockplus.org/6569732794744832/diff/5629499534213120/kick.py File kick.py (right): http://codereview.adblockplus.org/6569732794744832/diff/5629499534213120/kick... kick.py:5: import getopt getopt isn't being used any more. http://codereview.adblockplus.org/6569732794744832/diff/5629499534213120/kick... kick.py:10: parser = createArgumentParser(description=description, add_help=False) I don't think there is a point in reusing the parser creation, it merely obscures the logic. Arguments for kick.py and run.py are just different, that's due to the use cases being quite different. They should just create their own argument parser each. http://codereview.adblockplus.org/6569732794744832/diff/5629499534213120/kick... kick.py:19: ) If you are switching to argparse, then you should specify a longer version for these arguments as well. Meaning supporting --test and --quiet. Also a nit: the messages should probably start with a lower-case letter for consistency (the default message for the help option does). http://codereview.adblockplus.org/6569732794744832/diff/5629499534213120/kick... kick.py:22: 'hosts', metavar='<host|group>', nargs='+', Nit (here and elsewhere): we should use the usual argparse format for metavar - just 'host|group' rather than '<host|group>'. http://codereview.adblockplus.org/6569732794744832/diff/5629499534213120/run.py File run.py (right): http://codereview.adblockplus.org/6569732794744832/diff/5629499534213120/run.... run.py:23: This is just wrong... 1) This assumes add_help=False parameter, yet you require the caller to pass it in explicitly. 2) Why even do this? Just let argparse take care of it. This will be more consistent anyway - -? is usually used on Windows, Unix tools should support --help. http://codereview.adblockplus.org/6569732794744832/diff/5629499534213120/run.... run.py:27: description = "Run a command on the given hosts or groups of hosts" Nit: single quotes, for consistency. http://codereview.adblockplus.org/6569732794744832/diff/5629499534213120/run.... run.py:38: type=lambda value: hosts.update((value,)) Nit: I think this should be a list semantically - hosts.update([value])
Summary of updates since the previous patch-set: - Use list instead of tuple when updating target hosts - Replaced double quites with single ones - Remove <characters> around argument metavars - Start argument help text with lower-case letters - Introduce --long as aliases for -* - Use -t host|group instead of -h, to allow the builtin help messages - Remove getopt import from run.py
http://codereview.adblockplus.org/6569732794744832/diff/5629499534213120/kick.py File kick.py (right): http://codereview.adblockplus.org/6569732794744832/diff/5629499534213120/kick... kick.py:5: import getopt On 2015/04/07 14:58:04, Wladimir Palant wrote: > getopt isn't being used any more. Done. http://codereview.adblockplus.org/6569732794744832/diff/5629499534213120/kick... kick.py:10: parser = createArgumentParser(description=description, add_help=False) On 2015/04/07 14:58:04, Wladimir Palant wrote: > I don't think there is a point in reusing the parser creation, it merely > obscures the logic. Arguments for kick.py and run.py are just different, that's > due to the use cases being quite different. They should just create their own > argument parser each. While I understand your point (and in fact had the same concern), I believe it is actually quite useful: The new parameters (in the next patch-set) and their handling won't be too trivial, and we of course do not want to repeat ourselves here. Sure, there are other implementation options, but IMO this is the cleanest one - and one of the shortest. http://codereview.adblockplus.org/6569732794744832/diff/5629499534213120/kick... kick.py:19: ) On 2015/04/07 14:58:04, Wladimir Palant wrote: > If you are switching to argparse, then you should specify a longer version for > these arguments as well. Meaning supporting --test and --quiet. > > Also a nit: the messages should probably start with a lower-case letter for > consistency (the default message for the help option does). Done. http://codereview.adblockplus.org/6569732794744832/diff/5629499534213120/kick... kick.py:22: 'hosts', metavar='<host|group>', nargs='+', On 2015/04/07 14:58:04, Wladimir Palant wrote: > Nit (here and elsewhere): we should use the usual argparse format for metavar - > just 'host|group' rather than '<host|group>'. Done. http://codereview.adblockplus.org/6569732794744832/diff/5629499534213120/run.py File run.py (right): http://codereview.adblockplus.org/6569732794744832/diff/5629499534213120/run.... run.py:23: On 2015/04/07 14:58:04, Wladimir Palant wrote: > This is just wrong... > > 1) This assumes add_help=False parameter, yet you require the caller to pass it > in explicitly. > > 2) Why even do this? Just let argparse take care of it. This will be more > consistent anyway - -? is usually used on Windows, Unix tools should support > --help. I just tried to stay consistent with the existing arguments for now. But yes this makes things more complicated. Thus I've now removed these kludges, so the default handling can apply again. Though that required re-naming '-h' into '-t' and '--target' (in run.py). http://codereview.adblockplus.org/6569732794744832/diff/5629499534213120/run.... run.py:27: description = "Run a command on the given hosts or groups of hosts" On 2015/04/07 14:58:04, Wladimir Palant wrote: > Nit: single quotes, for consistency. Done. http://codereview.adblockplus.org/6569732794744832/diff/5629499534213120/run.... run.py:38: type=lambda value: hosts.update((value,)) On 2015/04/07 14:58:04, Wladimir Palant wrote: > Nit: I think this should be a list semantically - hosts.update([value]) Done.
http://codereview.adblockplus.org/6569732794744832/diff/5724160613416960/kick.py File kick.py (right): http://codereview.adblockplus.org/6569732794744832/diff/5724160613416960/kick... kick.py:56: if __name__ == "__main__": Nit: We should use single quotes here as well if you are fixing it everywhere. http://codereview.adblockplus.org/6569732794744832/diff/5724160613416960/run.py File run.py (right): http://codereview.adblockplus.org/6569732794744832/diff/5724160613416960/run.... run.py:14: '-u', '--user', metavar='<user>', dest='user', type=str, default='vagrant', Please remove < and > from metavar here as well.
...
http://codereview.adblockplus.org/6569732794744832/diff/5724160613416960/kick.py File kick.py (right): http://codereview.adblockplus.org/6569732794744832/diff/5724160613416960/kick... kick.py:56: if __name__ == "__main__": On 2015/04/07 15:50:25, Wladimir Palant wrote: > Nit: We should use single quotes here as well if you are fixing it everywhere. Done. http://codereview.adblockplus.org/6569732794744832/diff/5724160613416960/run.py File run.py (right): http://codereview.adblockplus.org/6569732794744832/diff/5724160613416960/run.... run.py:14: '-u', '--user', metavar='<user>', dest='user', type=str, default='vagrant', On 2015/04/07 15:50:25, Wladimir Palant wrote: > Please remove < and > from metavar here as well. Done.
LGTM 
|
