-
Notifications
You must be signed in to change notification settings - Fork 40
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
ion-resource: add meaningful error message when subcommand is missing #1243
ion-resource: add meaningful error message when subcommand is missing #1243
Conversation
Problem: a subcommand "func" is always required for ion resource, yet the current client spits out an error if the user types the command without help, subcommand, or other args. Solution: if the func (subcommand) is missing, print the parser help and provide a meaningful message to the user. Signed-off-by: vsoch <[email protected]>
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.
Great catch! LGTM. Argparse has a way to support this with the required=True
argument to add_subparsers
but it isn't added until 3.7 :( https://docs.python.org/3/library/argparse.html#argparse.ArgumentParser.add_subparsers otherwise we could have done
diff --git a/src/cmd/flux-ion-resource.py b/src/cmd/flux-ion-resource.py
index 8fa71ef1..1a1a7e22 100755
--- a/src/cmd/flux-ion-resource.py
+++ b/src/cmd/flux-ion-resource.py
@@ -411,7 +411,10 @@ def parse_match(parser_m: argparse.ArgumentParser):
"""
subparsers_m = parser_m.add_subparsers(
- title="Available Commands", description="Valid commands", help="Additional help"
+ title="Available Commands",
+ description="Valid commands",
+ help="Additional help",
+ required=True,
)
Thanks @jameshcorbett ! I think I almost like this way better, regardless of version, because it allows us to give the message to the user. If we just spit out the usage it could only be inferred something is wrong, but not specifically what. |
@jameshcorbett what does an ion resource refer to? I keep thinking of iron man. Iron man is pretty cool so I can't say this is a bad thing, but I'm wondering what an "ion resource" actually is. Is it a reference to ion and if so, what is the relation to our fluxion graphs? |
That's fair, although I think the way
AFAIK all fluxion commands are prefixed with |
Ahh of course! And I totally missed that. I think (when I read verbatim) I miss it and wonder what a flux ion is. But that's probably just me - it took me over a year to figure out that flux-sched was the same thing as fluxion. Thank you for the review! |
You should feel free to set MWP unless you want another review! |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #1243 +/- ##
======================================
Coverage 74.4% 74.4%
======================================
Files 104 104
Lines 14936 14936
======================================
Hits 11118 11118
Misses 3818 3818 |
Problem: a subcommand "func" is always required for ion resource, yet the current client spits out an error if the user types the command without help, subcommand, or other args.
Solution: if the func (subcommand) is missing, print the parser help and provide a meaningful message to the user.
Details
I am starting to test out flux ion-resource (to learn about interaction with fluxion and the queue manager) and very naively I built and ran the command and saw this:
It's pretty common for the user to try out a command by typing it first, with or without help, and expecting it to dump out usage. So that's what I've updated it to do here. The "func" argument that is missing is a subcommand, so we can check if the attribute is defined on the parser, and if not, print the parser help, and a clear message to the user that a subcommand is required. Here is the new interaction:
That should be a fairly straight forward update if the fix is desired. Note that I have not tested nothing else yet - I am going to bed so likely won't until tomorrow. Looking forward to trying it out!