feat(mobile-app): Make missing path not panic#2583
Conversation
rbro112
changed the title
| is required for mobile app uploads. {SELF_HOSTED_ERROR_HINT}" | ||
| ) | ||
| })?; | ||
| ) |
There was a problem hiding this comment.
Looks like an autoformat update.
src/commands/mobile_app/upload.rs
Outdated
| None => { | ||
| eprintln!("path argument is required"); | ||
| return Ok(()); |
There was a problem hiding this comment.
We should instead return an error so that we exit with nonzero status
There was a problem hiding this comment.
I took this impl from the proguard mapping upload, will update but notable that that command does not return an error.
src/commands/mobile_app/upload.rs
Outdated
| let path_strings: Vec<_> = match matches.get_many::<String>("paths") { | ||
| Some(paths) => paths.collect(), | ||
| None => { | ||
| return Err(anyhow!("path argument is required")); |
There was a problem hiding this comment.
Sorry I did not realize this earlier in the review, but I believe there should be a way to get clap to enforce the path argument as required?
If you cannot get it to work, I'd try to write a better error message, as path is a positional argument, and as written, the error message could be confusing to users since there is no --path argument.
Something like "you must provide at least one path to upload" I think would be better
szokeasaurusrex
left a comment
szokeasaurusrex
left a comment
There was a problem hiding this comment.
See comment before merge, looks good otherwise!
| let path_strings = matches | ||
| .get_many::<String>("paths") | ||
| .expect("paths argument is required"); | ||
| let path_strings: Vec<_> = match matches.get_many::<String>("paths") { | ||
| Some(paths) => paths.collect(), | ||
| None => { | ||
| // Clap will handle the case where no paths are provided. | ||
| return Ok(()); | ||
| } | ||
| }; |
There was a problem hiding this comment.
Since clap is handling this already (or at least, as it should be), I would go back to the expect (which panics) since getting into the state where we don't have paths at this point is unexpected and would indicate a bug in Sentry CLI. You can update the expect message to state that Clap is enforcing that the argument is required.
2 participants
Updates the mobile-app upload command to no longer panic upon a missing path argument, rather log an error and exit gracefully. Adds a test to validate this behavior.