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

Showing help when no input parameters are given and suppress warnings for cmds #1523

Merged
merged 7 commits into from Jul 9, 2018

Conversation

Projects
None yet
4 participants
@parichit
Copy link
Contributor

parichit commented May 15, 2018

This change is made in the base.py file to check if the user has provided any arguments or not.

Currently, if the workflows are run without any parameters then there is no help message to prompt the user of what went wrong.

Added the check and put an appropriate error message.

Parichit Sharma added some commits May 15, 2018

Parichit Sharma
DIPY workflows: Added a check for finding if the user did not provide…
… any arguments.

1) Added a simple check in the the parse_args() function to find if no arguments were supplied.
2) If no arguments were provided then a help message is printed and the program exists (using simple exit(1)).
3) If arguments were provided then the program resumes normal execution.
Parichit Sharma
@pep8speaks

This comment has been minimized.

Copy link

pep8speaks commented May 15, 2018

Hello @parichit, Thank you for updating !

Cheers ! There are no PEP8 issues in this Pull Request. 🍻

Comment last updated on July 02, 2018 at 21:07 Hours UTC

@skoudoro skoudoro added the gsoc2018 label May 15, 2018

@codecov-io

This comment has been minimized.

Copy link

codecov-io commented May 16, 2018

Codecov Report

Merging #1523 into master will decrease coverage by 0.13%.
The diff coverage is 66.66%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1523      +/-   ##
==========================================
- Coverage   87.55%   87.42%   -0.14%     
==========================================
  Files         246      246              
  Lines       31366    31461      +95     
  Branches     3433     3425       -8     
==========================================
+ Hits        27464    27504      +40     
- Misses       3085     3143      +58     
+ Partials      817      814       -3
Impacted Files Coverage Δ
dipy/workflows/base.py 79.56% <ø> (ø) ⬆️
dipy/fixes/argparse.py 35.33% <0%> (-0.04%) ⬇️
dipy/workflows/flow_runner.py 97.43% <100%> (+0.13%) ⬆️
dipy/tracking/streamline.py 67.68% <0%> (-28.35%) ⬇️
dipy/reconst/forecast.py 90.15% <0%> (-2.08%) ⬇️
dipy/core/graph.py 73.8% <0%> (-1.2%) ⬇️
dipy/align/imwarp.py 98.39% <0%> (-0.23%) ⬇️
dipy/tracking/tests/test_streamline.py 98.25% <0%> (ø) ⬆️
dipy/viz/interactor.py 98.15% <0%> (+0.03%) ⬆️
dipy/reconst/mapmri.py 90.93% <0%> (+0.64%) ⬆️
... and 2 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update f4c7e45...10b0d8f. Read the comment docs.

Parichit Sharma
Modifed the check for empty arguments in the base.py file.
1) Thanks @serge for pointing out this. I have updated the if condition to now check if the args string is None. It was checking the length of args earlier.
2) Replaced the  sys.argv[0] by self.prog to maintain code consistency.
3) Manual code debugging revealed that the condition is now being executed.
@parichit

This comment has been minimized.

Copy link
Contributor

parichit commented May 16, 2018

Thanks @serge for pointing out the PEP8 issue. I updated the length of the code (in the base.py file) to conform to the PEP8 formatting style. The PEP8 issue is now resolved.

I am waiting for the Travis-ci checks to complete now.

Parichit Sharma added some commits May 17, 2018

Parichit Sharma
Code reshuffle and updation
Case1: When user does not provide any arguments

1) Removed the code from parse_args() function (in base.py file) to _parse_known_arguments() fuction (in the argparse.py file).

2) The earlier code was not consistent for all workflows. It was only checking if the args value is None and reporting the error based on that. That logic will not work since simply giving -h/--help parameter will also trigger the error since args would still be None in that case but we are actually asking for the help and not the error message.
3) Current code works for all the workflows except the dipy_info workflow.
4) Tested the changes by running the workflows manually on the commandline with and without any parameters.
5) A user friendly message is displayed along with the error trace now. Previously only the error trace was
reported without any helpful message to the user.

Future Work
1) Have to discuss about the dipy_info workflow and why the help message is not displayed for it.

Case2: Disabling the FutureWarnings from h5py

1) Added the code to disable only the FutureWarning in the flow_runner.py file. Since, the runflow method is called by all the workflows so disabling of warning is done inside the flow_runner.py file.
2) Now the absurd FutureWarning is disabled and no longer displayed.
3) Tested the change manually by running the workflows on the command line and no warning was reported.

@Garyfallidis Garyfallidis changed the title Troubleshoot branch parichit Troubleshoot branch May 22, 2018

@Garyfallidis Garyfallidis changed the title Troubleshoot branch Showing help when no input parameters are given and suppress warnings for cmds May 22, 2018

@@ -1909,6 +1909,10 @@ def consume_positionals(start_index):
# if we didn't use all the Positional objects, there were too few
# arg strings supplied.
if positionals:
# printing user friendly help message to tell about missing
# arguments.
print("Too few arguments. Program", self.prog, "expects arguments"

This comment has been minimized.

@skoudoro

skoudoro Jul 2, 2018

Member

Can you add some "\n\n"

This comment has been minimized.

@parichit

parichit Jul 2, 2018

Contributor

@skoudoro: Thanks for pointing this out. I have added the line gaps and committed. Thanks.

@skoudoro

This comment has been minimized.

Copy link
Member

skoudoro commented Jul 3, 2018

ok, LGTM, I will merge tomorrow if there is no other comment

@skoudoro skoudoro merged commit e17a7ea into nipy:master Jul 9, 2018

1 check failed

continuous-integration/travis-ci/pr The Travis CI build failed
Details

ShreyasFadnavis pushed a commit to ShreyasFadnavis/dipy that referenced this pull request Sep 20, 2018

Merge pull request nipy#1523 from parichit/troubleshoot_branch_parichit
Showing help when no input parameters are given and suppress warnings for cmds
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment