Ensure that -list-labels is the last argument that is parsed
#3864
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This ensures that other arguments are parsed first, so that `-list-labels` can see those parsed values. Conversely, if `-list-labels` were to be parsed first instead, then it wouldn't see any of the updates to default argument values.
joshuacwnewton
added
bug
mguaypaq
approved these changes
Aug 22, 2022
There was a problem hiding this comment.
At first I thought this fix didn't feel right (messing around with the strings in argv), and that -list-labels should just have been processed once all arguments are parsed. But as documented in _ListLabelsAction (yay docs!), parsing would fail first because of a missing mandatory argument -i. (And a custom action that calls parser.exit() is how argparse itself handles the --help argument, so that has to be acceptable.)
Which is a long-winded way of saying that this is fine! Thanks for the fix.
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
sct_extract_metric -list-labelsis a way to display theinfo_label.txtfile contained within the folder pointed to by-f:spinalcordtoolbox/spinalcordtoolbox/scripts/sct_extract_metric.py
Line 51 in e7b423e
However, if the user specifies a new
-f, then the order of parsing starts to matter:sct_extract_metric -f <directory> -list-labelsis used, then-fwill be updated first, and-list-labelswill display correctly.sct_extract_metric -list-labels -f <directory>is used instead, then-list-labelswill be parsed before-fcan be updated, and it will either crash, or display the wronginfo_label.txtfile.To make sure that
-list-labelsalways works as expected, we need to parse it last in the list of arguments, hence this PR.Linked issues
Fixes #3634.