Skip to content

Accept only one argument when deleting a gist#3021

Merged
mislav merged 5 commits intocli:trunkfrom
g14a:bug/gist-deletion
Feb 24, 2021
Merged

Accept only one argument when deleting a gist#3021
mislav merged 5 commits intocli:trunkfrom
g14a:bug/gist-deletion

Conversation

@g14a
Copy link
Contributor

@g14a g14a commented Feb 23, 2021

This commit adds a validation to check if multiple gist urls/ids are passed during deletion. It accepts only one url/id. Ref. #3015

Copy link
Contributor

@mislav mislav left a comment

Choose a reason for hiding this comment

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

Thank you! This looks good, but I'm wondering whether we can do a tiny bit better! When no arguments are passed to gist delete, we now lose the descriptive error message cannot delete: gist argument required. Furthermore, there are more commands in our codebase that use cmdutil.MinimumArgs when they in fact want to verify that exactly 1 argument is required.

What do you think about creating a new helper cmdutil.ExactArgs(num, message) that would print the message if there aren't enough arguments and print something like "too many arguments" when the argument length exceeds num? In both cases, the returned error type should be of the type *cmdutil.FlagError.

Then, you could go over current usage of cmdutil.MinimumArgs and check whether that should be switched over to ExactArgs instead.

@g14a
Copy link
Contributor Author

g14a commented Feb 23, 2021

Sure let me mull over it.

@g14a
Copy link
Contributor Author

g14a commented Feb 24, 2021

@mislav I thought the ExactArgs function would really be helpful in showing verbose errors. I've replaced MinimumArgs at places with ExactArgs where it seemed more appropriate and updated the PR.

Copy link
Contributor

@mislav mislav left a comment

Choose a reason for hiding this comment

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

Thank you! This looks great; just minor corrections needed

@g14a
Copy link
Contributor Author

g14a commented Feb 24, 2021

Thank you! This looks great; just minor corrections needed

@mislav added the corrections

Copy link
Contributor

@mislav mislav left a comment

Choose a reason for hiding this comment

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

This looks great! Thank you

@mislav mislav merged commit 896f227 into cli:trunk Feb 24, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants