-
Notifications
You must be signed in to change notification settings - Fork 41
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Inspect sys.argv for --projects with argparse before click #283
Inspect sys.argv for --projects with argparse before click #283
Conversation
Codecov Report
@@ Coverage Diff @@
## master #283 +/- ##
==========================================
+ Coverage 99.31% 99.32% +<.01%
==========================================
Files 52 52
Lines 2624 2651 +27
==========================================
+ Hits 2606 2633 +27
Misses 18 18
Continue to review full report at Codecov.
|
This was for #210 Some thoughts: It'd be better to be able to use Click. I'm a bit nervous about combining two different ways of processing command line arguments. For one thing, Click generates nice help texts. Argparse does that too, but both cannot work at the same time. After this change,
The order of parameters is not crucial. The
But the initialization order is difficult. Right now the value is passed via It seems to me that the first time the projects file is actually needed is when |
This reverts commit 2cf6b2a.
…option-path-to-projects-file
Code-wise ready for review. I'm not really sure what to do with the failing quality checks, I'll look at the failed Drone build (it should not even be building for this). |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Looks very good.
The Code Climate complaints can be ignored. I think that in general this PR improves the overall code quality, thanks to the @common_options
decorator, despite the small additional complexity it introduces.
The test should be split into two separate functions, but other than that, this is OK for merging.
assert 'Project configuration file "{}" is missing.'.format( | ||
nonexistent_file) in result.output | ||
|
||
result = runner.invoke( |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is testing a different situation (existing config file), so it should be separated into its own test.
This is just first trial, here's some thoughts:
At first the best way to set the path to
projects.cfg
seemed to use some click decorator, but the first problem with this approach is that the cli methods are defined only after the app is already created in https://github.com/NatLibFi/Annif/blob/master/annif/cli.py#L22:cli = FlaskGroup(create_app=annif.create_app)
Also I could not yet find a way to add an option to be used directly after
annif
, i.e. which would apper along--version
and--help
in help:But this is possible, https://kite.com/blog/python/python-command-line-click-tutorial, I just can't get it work in this case when
FlaskGroup
is involved.So for now, this first attempt inspects
sys.argv
withargparse
for--projects
and uses its value to set the env ANNIF_PROJECTS which in turn sets the path toprojects.cfg
. This works, butargparse
just for the--projects
flag, whenclick
is used for other cli cases--version
and--help
)Maybe the
FlaskGroup
should be wrapped in custom class as in https://stackoverflow.com/questions/44056153/optional-argument-in-command-with-click