-
Notifications
You must be signed in to change notification settings - Fork 18
Edge cases and error handling for the squeeze morph #227
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
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #227 +/- ##
==========================================
+ Coverage 99.04% 99.07% +0.02%
==========================================
Files 21 21
Lines 1048 1079 +31
==========================================
+ Hits 1038 1069 +31
Misses 10 10
🚀 New features to boost your workflow:
|
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.
one small question
"1,a,0", | ||
] | ||
) | ||
with pytest.raises(SystemExit): |
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.
why are we using sys.exit and not a proper exception? In general, this is to be avoided as it bypasses all the nice exception handling you get for free in Python?
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 a test for the CLI, and the intended behavior is that once we receive a ValueError from the 'a' character, the parser raises an error, and the CLI program exits with value 1
.
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.
The line parser.error(f"{coeff} could not be converted to float.")
(line 549 on 'morphapp.py') is calling the in-built option parser function to handle the error once we detect a ValueError on the Python side.
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.
Hmm, but we should ensure that the test is capturing this particular error, and not necessarily system exiting elsewhere. Let me put a new commit in a bit.
I am happy to merge this but it is a bit overly complicated I think. We generally don't test the parser or handle improper inputs beyond what is native to argparse. That's fine though..... |
Throw error when invalid input is given (e.g. non-numeric list).
Indicate to user that duplicated/repeated commas and trailing commas will be removed: e.g.
0,,1,
becomes0,1
.