feat(http): add --verbose / -v flag for request/response diagnostics#486
feat(http): add --verbose / -v flag for request/response diagnostics#486abhiram304 wants to merge 2 commits intogoogleworkspace:mainfrom
Conversation
🦋 Changeset detectedLatest commit: 33d885b 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 introduces a new global Highlights
Changelog
Activity
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. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
There was a problem hiding this comment.
Code Review
This pull request introduces a new global --verbose (-v) flag to the CLI, enabling detailed logging of HTTP requests and responses (method, URL, status, timing) to stderr for debugging purposes. It also updates the --format option to include tsv as a valid output format. The changes include adding the flag definition, modifying the execute_method signature to accept the verbose flag, and implementing the conditional logging logic. Review comments highlight that the verbose output for request URLs is incomplete, as it currently omits query parameters and pageToken for paginated requests, which could be misleading. Additionally, several helper commands (calendar, chat, docs, drive, gmail, script, sheets) hardcode the verbose argument to false when calling execute_method, preventing the global --verbose flag from functioning correctly within these contexts.
Adds a global --verbose / -v flag that prints HTTP request and response details to stderr without touching stdout, keeping pipe-friendly output intact. When enabled: > GET https://www.googleapis.com/drive/v3/files < 200 OK (143ms) [page 2] ← emitted at the start of each additional page fetch Implementation: - commands.rs: declare --verbose as a global SetTrue arg (mirrors --dry-run) - executor.rs: add verbose: bool parameter to execute_method(); log method + URL before send(), status + elapsed after; log [page N] at top of loop for pages > 1. Also adds stdout/stderr contract doc comment. - main.rs: extract get_flag("verbose") and thread it through - All 8 helper execute_method() call sites: pass false (helpers have no --verbose wiring yet; callers that want it can opt in later)
4f8a0cc to
2f76224
Compare
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a valuable --verbose flag for HTTP request diagnostics, which will greatly aid in debugging. The implementation is well-structured, following existing patterns in the codebase, and includes appropriate tests. I've identified two high-severity issues. One is a correctness issue where the URL printed in verbose mode is not properly encoded, which could be misleading. The other is that the help text for the --format option has been updated to include tsv, but this format is not actually implemented, which will confuse users and is also out of scope for this PR, violating the rule against scope creep. My review provides specific suggestions to address these points.
Address review feedback: - Use url::form_urlencoded for proper percent-encoding of query params in verbose diagnostic output, matching what reqwest actually sends - Revert --format help text to "json, table, yaml, csv" — tsv mention was out of scope for this PR (tracked separately in feat/add-tsv-format) - Add `url` as an explicit dependency (was previously transitive-only)
|
/gemini review |
There was a problem hiding this comment.
Code Review
This pull request introduces a --verbose / -v flag for HTTP request/response diagnostics, which is a great feature for debugging. The implementation correctly adds the global flag and passes it down to the executor. The new logging in executor.rs provides useful information to stderr. However, I've identified a high-severity issue with code duplication: the logic to construct the URL for logging duplicates the query parameter assembly logic from build_http_request. This could lead to inconsistencies in the future. My review includes a comment with a suggestion for refactoring to improve maintainability.
Summary
--verbose/-vflag that prints HTTP method, URL, response status, and elapsed time to stderr on every request — stdout remains clean JSON sogws ... | jqpipelines are unaffected--dry-runflag: declared global incommands.rs, extracted inmain.rs, threaded throughexecute_method()asverbose: bool[page N]is emitted to stderr before each page fetch beyond the firststd::time::Instant— no new dependenciesSample output (
--verbose --dry-run):Test plan
gws drive files list --verbose --dry-run— stderr shows> GET https://..., stdout is the dry-run JSON objectgws drive files list --verbose --dry-run 2>/dev/null | jq .— jq receives valid JSON-vshort form works identically to--verbosefalseat all call sites)cargo test commands—test_verbose_arg_present_and_globalpassescargo test executor— updated dry-run tests compile and pass🤖 Generated with Claude Code