danilobellini / dose

Traffic light/signal/semaphore GUI showing the result of a test command job triggered by a file modification
GNU General Public License v3.0
44 stars 8 forks source link

Add support for dose specific cli args #10

Closed samuelgrigolato closed 3 months ago

samuelgrigolato commented 7 years ago

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

samuelgrigolato commented 7 years ago

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

samuelgrigolato commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

I did a little more work on this code:

danilobellini commented 7 years ago

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 commented 7 years ago

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.