feat(mobile-app): Add new VCS params to mobile-app command#2682
feat(mobile-app): Add new VCS params to mobile-app command#2682
Conversation
rbro112
changed the title
rbro112
force-pushed
the
ryan/add_new_vcs_params_to_mobile-app_command
branch
from
dd65d3b to
38b2596
Compare
src/commands/mobile_app/upload.rs
Outdated
| .map(Cow::Borrowed) | ||
| .or_else(|| vcs::find_head().ok().map(Cow::Owned)); | ||
|
|
||
| // TODO: Implement default values |
There was a problem hiding this comment.
Will follow in future PRs for each of these individually.
There was a problem hiding this comment.
I am okay with us doing this in future PRs; however, I would prefer not to commit the TODO comment, lest we ever forget to remove it. We should track the work in GitHub/Linear issues instead
| // TODO: Implement default values |
![cursor[bot]](https://avatars.githubusercontent.com/in/1210556?s=60&v=4){kind=small}
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
This comment was marked as outdated.
This comment was marked as outdated.
Sorry, something went wrong.
szokeasaurusrex
left a comment
szokeasaurusrex
left a comment
There was a problem hiding this comment.
lgtm, besides a few minor suggestions to address prior to merge
| #[serde(skip_serializing_if = "Option::is_none")] | ||
| pub base_ref: Option<&'a str>, | ||
| #[serde(skip_serializing_if = "Option::is_none")] | ||
| pub pr_number: Option<i32>, |
There was a problem hiding this comment.
As PR's can only have positive numbers, I would use an unsigned integer (u32) here, rather than a signed integer
| pub pr_number: Option<i32>, | |
| pub pr_number: Option<u32>, |
src/commands/mobile_app/upload.rs
Outdated
| .map(Cow::Borrowed) | ||
| .or_else(|| vcs::find_head().ok().map(Cow::Owned)); | ||
|
|
||
| // TODO: Implement default values |
There was a problem hiding this comment.
I am okay with us doing this in future PRs; however, I would prefer not to commit the TODO comment, lest we ever forget to remove it. We should track the work in GitHub/Linear issues instead
| // TODO: Implement default values |
| Arg::new("pr_number") | ||
| .long("pr-number") | ||
| .help("The pull request number to use for the upload. If not provided, the current pull request number will be used.") |
There was a problem hiding this comment.
This way, Clap can parse straight to a u32 and perform the validation internally
| Arg::new("pr_number") | |
| .long("pr-number") | |
| .help("The pull request number to use for the upload. If not provided, the current pull request number will be used.") | |
| Arg::new("pr_number") | |
| .long("pr-number") | |
| .value_parser(clap::value_parser!(u32)) | |
| .help("The pull request number to use for the upload. If not provided, the current pull request number will be used.") |
There was a problem hiding this comment.
Very nice to know, figured there was a way to do this I was missing
src/commands/mobile_app/upload.rs
Outdated
| let pr_number = matches | ||
| .get_one("pr_number") | ||
| .map(String::as_str) | ||
| .and_then(|s| s.parse::<i32>().ok()); |
There was a problem hiding this comment.
Once you change the pr_number's type to u32 in the struct, and once you add the value_parser above, you will be able to simplify this line greatly:
| let pr_number = matches | |
| .get_one("pr_number") | |
| .map(String::as_str) | |
| .and_then(|s| s.parse::<i32>().ok()); | |
| let pr_number = matches.get_one("pr_number"); |
rbro112
force-pushed
the
ryan/add_new_vcs_params_to_mobile-app_command
branch
from
e9979f6 to
c5ecf09
Compare
3 participants
Adds new VCS params as args to the
mobile-appcommand and passes them to the API (pending getsentry/sentry#97332). Notably removesshain favor ofhead_sha. Given this command is still unreleased and marked experimental, this would usually be a breaking change but I don't believe we need any backwards compatibility here.We'll implement default value providing for most of these as well in follow ups, as I'd prefer to silo those implementations for easier review && testing.