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

Used pattern matching to match subcommands #4

Merged
merged 1 commit into from
Mar 18, 2021

Conversation

vedangj044
Copy link
Contributor

Fixes #3

  • The panic caused by unwraps is handling using pattern matching.
  • required(true) is added to app_id_arg to delegate error handling to clap.

@vedangj044
Copy link
Contributor Author

The following things can be improved in this PR, need your suggestions

  • The matches.subcommand() returns (&str, <options>), can we convert enums to string. I was not able to use Verbs::create.as_ref() or format!(Verbs::create.as_ref()) as a match expression.
  • Use of enums is a discussed issue in the clap repository as well, maybe we can implement this in the future. See here.

src/main.rs Outdated Show resolved Hide resolved
src/main.rs Outdated Show resolved Hide resolved
@jbtrystram
Copy link
Member

The following things can be improved in this PR, need your suggestions

* The `matches.subcommand()` returns `(&str, <options>)`, can we convert enums to string. I was not able to use `Verbs::create.as_ref()` or `format!(Verbs::create.as_ref())` as a match expression.

* Use of enums is a discussed issue in the clap repository as well, maybe we can implement this in the future. [See here.](https://github.com/clap-rs/clap/issues/459)

clap-rs/clap#1104 this issue seems to be stalled for a few years now, so i would not count on it ! It's nice if it finally lands, but in the meantime i'd like to keep the enums and find a workaround.

@vedangj044
Copy link
Contributor Author

Actually, that issue was the first time they discussed it, they have added an example about the use of enums here. Our current implementation is good too, so I think we keep it this way for now.

@jbtrystram
Copy link
Member

Can you explain a bit more the issue you have with matching the commands with the enums variants ?

@jbtrystram
Copy link
Member

I already used drang for converting the enums variants to string. Maybe you just need to import asRefStr in the main file and use it ?
https://docs.rs/strum/0.20.0/strum/derive.AsRefStr.html

src/main.rs Outdated Show resolved Hide resolved
src/main.rs Outdated Show resolved Hide resolved
Copy link
Member

@jbtrystram jbtrystram left a comment

Choose a reason for hiding this comment

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

See comments. Also please run cargo fmt on your PRs

src/main.rs Outdated Show resolved Hide resolved
src/arguments.rs Outdated
@@ -64,9 +65,11 @@ App::new("Drogue Command Line Tool")
.about("Allows to manage drogue apps and devices in a drogue-cloud instance")
.arg(config_file_arg)
.arg(url_arg)
.setting(AppSettings::SubcommandRequiredElseHelp)
Copy link
Member

Choose a reason for hiding this comment

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

Why not adding : setting(AppSettings::SubcommandRequiredElseHelp) to the App object at the root level instead of adding it to every subcommand ?
(maybe it does not works recursively, but i think it does at it is not mentionned in the documentation)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Tried this but it doesn't work recursively.

@vedangj044 vedangj044 force-pushed the main branch 2 times, most recently from fa4ef28 to 291c5a0 Compare March 18, 2021 12:11
Added argsElseHelp flag

formatted code

Fixed typo
@jbtrystram jbtrystram merged commit 6c78fa0 into drogue-iot:main Mar 18, 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.

Fixing unsafe unwraps in subcommand
2 participants