fix(sheets): require values for append helper#787
fix(sheets): require values for append helper#787rohan-patnaik wants to merge 3 commits intogoogleworkspace:mainfrom
Conversation
🦋 Changeset detectedLatest commit: 33c3c34 The changes in this PR will be included in the next version bump. This PR includes changesets to release 1 package
Not sure what this means? Click here to learn what changesets are. Click here if you're a maintainer who wants to add another changeset to this PR |
Summary of ChangesHello, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request improves the reliability of the Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize the Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counterproductive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for GitHub and other Google products, sign up here. Footnotes
|
googleworkspace-bot
added
the
area: core
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request ensures that row data is provided when using the gws sheets +append command by making the --values and --json-values arguments required unless the other is present. It also includes a unit test to verify this behavior. The review feedback recommends explicitly marking these arguments as conflicting to prevent ambiguity if both are supplied simultaneously.
| Arg::new("values") | ||
| .long("values") | ||
| .help("Comma-separated values (simple strings)") | ||
| .value_name("VALUES"), | ||
| .value_name("VALUES") | ||
| .required_unless_present("json-values"), | ||
| ) | ||
| .arg( | ||
| Arg::new("json-values") | ||
| .long("json-values") | ||
| .help("JSON array of rows, e.g. '[[\"a\",\"b\"],[\"c\",\"d\"]]'") | ||
| .value_name("JSON"), | ||
| .value_name("JSON") | ||
| .required_unless_present("values"), | ||
| ) |
There was a problem hiding this comment.
The --values and --json-values arguments are mutually exclusive ways to provide row data. Currently, they are marked as required_unless_present, which ensures at least one is provided, but it allows both to be present simultaneously. If both are provided, the parsing logic in parse_append_args silently prioritizes --json-values and ignores --values, which is ambiguous and can lead to unexpected behavior or data loss. Marking them as conflicting ensures the user provides exactly one valid input method.
Arg::new("values")
.long("values")
.help("Comma-separated values (simple strings)")
.value_name("VALUES")
.required_unless_present("json-values")
.conflicts_with("json-values"),
)
.arg(
Arg::new("json-values")
.long("json-values")
.help("JSON array of rows, e.g. '[(\"a\",\"b\"], [\"c\",\"d\"]]'")
.value_name("JSON")
.required_unless_present("values")
.conflicts_with("values"),
)
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request updates the gws sheets +append command to require that either the --values or --json-values argument is provided, while also ensuring they are mutually exclusive. New unit tests were added to verify these constraints. The review feedback suggests improving the test robustness by asserting the specific error kind when required arguments are missing, rather than just checking for the presence of any error.
| assert!( | ||
| result.is_err(), | ||
| "+append should require --values or --json-values" | ||
| ); |
There was a problem hiding this comment.
This test should verify the specific error kind (e.g., ErrorKind::MissingRequiredArgument) to ensure it fails for the expected reason. Without this check, the test could pass if the command fails for other reasons, such as a typo in the subcommand name or a missing required global argument. This also maintains consistency with the test_append_rejects_values_and_json_values_together test below.
| assert!( | |
| result.is_err(), | |
| "+append should require --values or --json-values" | |
| ); | |
| assert_eq!( | |
| result.unwrap_err().kind(), | |
| clap::error::ErrorKind::MissingRequiredArgument | |
| ); |
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request updates the gws sheets +append command to require either the --values or --json-values argument, while ensuring they are mutually exclusive. It also includes a changeset and unit tests to verify these new argument constraints. I have no feedback to provide.
|
Gemini feedback has been addressed in the follow-up commits:
All local validation passed with |
2 participants
Description
gws sheets +appendshould only run when there is row data to append. Previously, the helper accepted a command with only--spreadsheet, which built an empty append request.This PR makes
+appendrequire either--valuesor--json-valuesduring argument parsing, so users get a clear CLI error before any API request is prepared.Dry Run Output:
// Not applicable: this validates required input before request creation.Checklist:
AGENTS.mdguidelines (no generatedgoogle-*crates).cargo fmt --allto format the code perfectly.cargo clippy -- -D warningsand resolved all warnings. See testing note below.pnpx changeset) to document my changes.Testing
cargo fmt --all --checkcargo test -qcargo clippy -- -D warningsis blocked locally by a pre-existingscript.rsclippy warning on currentmain; that warning is fixed separately in fix(script): skip ignored directories during push #786.