fix(helpers): respect --format flag in sheets +append, +read, and docs +write#678
fix(helpers): respect --format flag in sheets +append, +read, and docs +write#678nuthalapativarun wants to merge 4 commits intogoogleworkspace:mainfrom
Conversation
🦋 Changeset detectedLatest commit: 8334dbf 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 |
|
Thanks for your pull request! It looks like this may be your first contribution to a Google open source project. Before we can look at your pull request, you'll need to sign a Contributor License Agreement (CLA). View this failed invocation of the CLA check for more information. For the most up to date status, view the checks section at the bottom of the pull request. |
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 addresses a functional inconsistency where specific CLI commands were defaulting to JSON output regardless of the user-provided format flag. By ensuring these commands properly propagate the requested output format to the underlying execution logic, the CLI now provides a consistent user experience across all supported operations. 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 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 counter productive. 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 aims to fix an issue where the --format flag was being ignored in the sheets and docs helpers. While the changes correctly pass an output_format to the executor, the current implementation incorrectly attempts to retrieve this global flag from the subcommand's ArgMatches due to variable shadowing. Feedback across all modified files suggests renaming the subcommand matches to avoid shadowing and retrieving the global --format flag from the parent ArgMatches to ensure it is correctly identified by clap v4.
|
bot: rescan |
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request ensures that the --format flag is respected in the sheets and docs helpers by retrieving the format from the command-line arguments and passing it to the executor. Feedback indicates that renaming the subcommand matches to sub_matches and using the parent matches for flag lookups prevents flags from being correctly resolved if provided after the subcommand. It is recommended to shadow the parent matches variable instead to ensure consistent flag resolution and avoid regressions for other flags like --dry-run.
…s +write These helpers hard-coded OutputFormat::default() (JSON) when invoking executor::execute_method, so --format table/yaml/csv had no effect. Each handler now reads the global --format flag from ArgMatches and passes it through, matching the behaviour of calendar +agenda and gmail +triage.
Rename the subcommand ArgMatches variable from `matches` to `sub_matches` so the outer `matches` is still in scope. Read the global --format flag from the parent matches rather than the subcommand matches, ensuring clap v4 correctly resolves the globally-defined argument.
nuthalapativarun
force-pushed
the
fix/helpers-format-flag
branch
from
fb21213 to
6dbe0ba
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request ensures that the --format flag is respected in the sheets +append, sheets +read, and docs +write helper commands by correctly extracting the global argument and passing it to the executor. Previously, these commands defaulted to JSON output regardless of the flag. I have no feedback to provide.
…mand ArgMatches In clap v4, global args propagate to subcommand ArgMatches, so shadowing the outer `matches` with the subcommand result is correct. Reading from the parent matches ignored flags placed after the subcommand (e.g. `gws sheets +append --format table`) and introduced a regression for dry-run lookups.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request fixes a bug where the --format flag was being ignored in the sheets +append, sheets +read, and docs +write helper commands. The implementation now correctly extracts the output format from the command-line arguments and passes it to the executor. Review feedback suggests avoiding the shadowing of the top-level matches variable with subcommand matches to ensure that global flags are reliably accessible, especially if they are not explicitly propagated by the argument parser.
…rent matches Avoid shadowing the parent `matches` with the subcommand result. Subcommand- specific args (passed to build_write_request, parse_append_args, parse_read_args) use `sub_matches`; global flags `--format` and `--dry-run` are read from the parent `matches` which is where they are defined and reliably present.
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request fixes an issue where the --format flag was being ignored in the sheets +append, sheets +read, and docs +write helper commands. The implementation now correctly extracts the global format argument and passes it to the executor, ensuring consistency with other commands. I have no feedback to provide.
2 participants
Description
sheets +append,sheets +read, anddocs +writeall hard-codedOutputFormat::default()(JSON) when callingexecutor::execute_method, meaning--format table,--format yaml, and--format csvhad no effect on their output.Each handler now reads the global
--formatflag fromArgMatchesand passes it through to the executor, matching the existing behaviour ofcalendar +agendaandgmail +triage.Example before fix:
# --format table was silently ignored; output was always JSON gws sheets +read --spreadsheet ID --range Sheet1 --format tableAfter fix: output respects the
--formatflag as with all other commands.Dry Run Output:
Checklist:
AGENTS.mdguidelines (no generatedgoogle-*crates).cargo fmt --allto format the code perfectly.cargo clippy -- -D warningsand resolved all warnings.pnpx changeset) to document my changes.