dose icon indicating copy to clipboard operation
dose copied to clipboard

Add support for dose specific cli args

Open samuelgrigolato opened this issue 8 years ago • 6 comments

Partial work for #6. Currently we have basic infrastructure for cli args parsing, and support for "--help" and "-h" switches.

samuelgrigolato avatar Oct 12 '16 22:10 samuelgrigolato

As a result of @danilobellini 's review, I'm going to do some changes on this code:

  • Move the args bootstrap from main_wx to main
  • Avoid the join/split args road trip
  • Ask forgiveness (via KeyError), not permission near if token in _supported_dose_switches:
  • It'd be better to use the term option instead of switch
  • Prepare support to GNU style parameters (multi one letter combined switches, i.e. -abc instead of -a -b -c)

samuelgrigolato avatar Oct 13 '16 00:10 samuelgrigolato

After some digging into click, it became clear that the work being done was not necessary and probably just reinventing the wheel. I'm proposing a new approach, a lot simpler and feature complete, based on the addition of click as a dependency. The most recent push into my cli-args branch is doing exactly that.

There's a problem, though: to be feature complete we need to break backward compatibility on some use cases: specifically, if you mix option-like arguments in your test_command (i.e. dose tox --something-like-that) it will trigger an exception and stop abruptly. Instead you should use this syntax to separate dose arguments from your test program arguments: dose -- tox --something-like-that. Note the double dash.

samuelgrigolato avatar Oct 30 '16 14:10 samuelgrigolato

Now we are warning the user instead of breaking backward compatibility. Whenever one try to use dose using old-style call syntax, we issue a console warning and bypass click's cli-args parsing decorator.

samuelgrigolato avatar Oct 30 '16 15:10 samuelgrigolato

I did a little more work on this code:

  • Updated CHANGES.rst
  • Fixed sys.argv dash testing conditional (startswith instead of index lookup)
  • Removed unnecessary _main delegate function created to allow decorator bypass. Click maintains a callback property in its decoration object that do the job cleanly

samuelgrigolato avatar Nov 02 '16 18:11 samuelgrigolato

You may leave the default prog_name as it is (which is probably sys.argv[0] removing the leading path and the trailing .py). Probably no one will ever call the __main__.py file directly as it would mess up with the paths, the dose directory itself is the "executable", and it will be mainly called by the dose script. The prog_name is probably seen only when calling Dose with dose --help, so I'd say it's not something to worry about (even if it's not so nice when calling Dose with python -m dose --help; hey, here's another arguments "bypasser" similar to dose and watch: the CPython executable itself).

OTOH, there might be a dose2 and dose3 scripts on some Linux distro to distinguish between Python 2 and 3 (I was thinking on making these myself on AUR, the Arch [Linux] User Repository), showing simply dose in the help would be misleading in those cases.

If you really wish to avoid using showing the __main__ as the program name in the help (due to the python -m dose actually showing __main__ instead of its directory name), you can use this:

if os.path.basename(sys.argv[0]) == "__main__.py":
    sys.argv[0] = os.path.dirname(os.path.abspath(sys.argv[0]))

Which gets the dose [package] directory instead of the __main__.py file. A directory with a __main__.py file is itself an executable that can be called with python -m dir_name:

https://docs.python.org/3/library/main.html https://docs.python.org/3/using/cmdline.html#cmdoption-m http://grokbase.com/t/python/docs/157epa1s9x/issue24632-improve-documentation-about-main-py

danilobellini avatar Nov 21 '16 04:11 danilobellini

I didn't understand the comments, the there is no need to re-quote sounds misleading. The watch command man page has just the opposite:

Note that command is given to "sh -c" which means that you may need to use extra quoting to get the desired effect.

Actually, that's exactly what Dose does (and should do) on Linux/MacOSX/Cygwin. Internal quoting/escaping can be required on a test command (e.g. when the executable name has a whitespace and there's no options/arguments).

In #11 I wrote some code to show that click can be used to parse the arguments using the POSIX option processing standard. That's enough to make -- optional and to add new options without breaking compatibility.

There are some coding style issues. Telling "otherwise" in a comment after an else is redundant, and there seem to be no need for any of those new comments (e.g. the callback is actually a documented method, and its name is a word that already tells us what's going on). Anyway, we won't need the callback method any longer if we use the POSIX option parsing scheme. Running tox fails on that code as there's a linter and a [quite permissive] style checker configured there that didn't like something in the code. Beyond the checker, I usually avoid using more "vertical" space than needed inside a function/method, such as small comments not inlined and empty lines on small code. I only use single empty lines inside a function/method when I think they're required for readability, e.g. when I want to seggregate blocks that have distinct ideas/steps of a process. There's nothing difficult on main, and using click will actually make its body simpler, you don't need to add any empty line there.

danilobellini avatar Nov 21 '16 05:11 danilobellini