Skip to content
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

Use SystemExit not sys.exit & only on error paths. #2983

Merged
merged 4 commits into from Oct 30, 2020

Conversation

kousu
Copy link
Contributor

@kousu kousu commented Oct 29, 2020

Suggestion for #2982

Copy link
Member

@joshuacwnewton joshuacwnewton left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like isct_convert_binary_to_trilinear -h fails because we didn't exit after the other usage() calls.

Traceback (most recent call last):
  File "/home/joshua/Repos/spinalcordtoolbox/scripts/isct_convert_binary_to_trilinear.py", line 196, in <module>
    main()
  File "/home/joshua/Repos/spinalcordtoolbox/scripts/isct_convert_binary_to_trilinear.py", line 92, in main
    check_file_exist(fname_data, verbose)
  File "/home/joshua/Repos/spinalcordtoolbox/spinalcordtoolbox/utils/fs.py", line 109, in check_file_exist
    if fname[0] == '-':
IndexError: string index out of range

scripts/isct_convert_binary_to_trilinear.py Show resolved Hide resolved
@kousu
Copy link
Contributor Author

kousu commented Oct 29, 2020

@joshuacwnewton i think i got it this time.
but btw i made this a PR into your PR so either you have to reenable yours and we need to do the two merges or I need to close this and resubmit it pointed at master. Can we do the former?

@joshuacwnewton
Copy link
Member

joshuacwnewton commented Oct 30, 2020

but btw i made this a PR into your PR so either you have to reenable yours and we need to do the two merges or I need to close this and resubmit it pointed at master. Can we do the former?

Terribly sorry for not noticing that! I'll rebase+merge and we can continue discussion there. :)

@joshuacwnewton joshuacwnewton merged commit d3b35d5 into jn/2981-remove-error-sys-exit Oct 30, 2020
@joshuacwnewton joshuacwnewton deleted the ng/2981-remove-sys-exit branch October 30, 2020 01:26
@jcohenadad jcohenadad added this to the 5.0.0 milestone Nov 6, 2020
@jcohenadad jcohenadad added the enhancement category: improves performance/results of an existing feature label Nov 6, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement category: improves performance/results of an existing feature
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants