Create standalone version of Get PR Comments script for use in other repos, with added features. #1742
Conversation
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
- Creates a new script `get_pr_review_comments_standalone.py`. - Inlines necessary functions from `firebase_github.py` to make it standalone. - Adds functionality to auto-determine GitHub owner/repo from git remote origin. - Allows specifying repository via `--url` or `--owner`/`--repo` arguments. - Prioritizes URL > owner/repo args > git remote detection. - Improves argument parsing and error handling for repository specification.
- Removes the deprecated nested `add_argument_group` for owner/repo. - `--owner` and `--repo` are now top-level arguments. - Implements manual validation logic after parsing to ensure correct argument pairing and exclusivity with `--url`. - This addresses the DeprecationWarning while maintaining the intended argument behavior.
- Makes the --pull_number argument optional. - If --pull_number is not provided, the script now attempts to: 1. Determine the current git branch. 2. Fetch open pull requests for this branch using the GitHub API. 3. Select the most recently created open PR. - If a PR cannot be determined either way, an error is shown. - Incorporates the `list_pull_requests` function from the original `firebase_github.py` to support this feature. - Adds error handling for git operations and API calls related to PR detection.
Removes the duplicate argparse definition for `--pull_number`, resolving the `argparse.ArgumentError`. The `--pull_number` argument is now correctly defined once as an optional argument, with the script falling back to auto-detection if it's not provided.
- Removes `parser.print_help()` calls for specific auto-detection failures: - When the repository owner/name cannot be determined automatically and is not specified by the user. - When the current git branch cannot be determined (and --pull_number is not provided). - The script will still print a targeted error message and exit with an error code in these scenarios. - Other error conditions (e.g., invalid explicit arguments, or no PR found for a valid detected branch) will continue to print full usage information.
- Removes `parser.print_help()` calls from all error exit paths related to missing or invalid script arguments. - Appends a consistent ' (use --help for more details)' note to these error messages. - This provides more concise error output while still guiding the user to help if needed. - Affects errors for token, URL, owner/repo specification, and PR number determination.
jonsimantov
commented
Jun 24, 2025
| ) | ||
|
|
||
| if not comments: | ||
| sys.stderr.write(f"No review comments found for PR #{pull_request_number} (or matching filters), or an error occurred.\n") |
There was a problem hiding this comment.
Please remove the note about an error occurring here, as it could lead the user to worry something went wrong. Just say there were no comments found. In fact, if get_pull_request_review_comments returned None if an error occurred and an empty list if there are simply no comments, you could just tell the difference between these two states and actually error out properly if an error did happen.
Performs a thorough pass on `get_pr_review_comments_standalone.py` to remove comments that are redundant, obvious restatements of code, obsolete placeholders, or no longer accurate due to prior changes. Retains comments that clarify complex or non-obvious logic, provide essential context, or document important decisions (such as compatibility shims or API-specific formatting). This change aims to improve code clarity and maintainability by reducing comment clutter.
Changes the default `absl.logging` verbosity level from INFO to WARNING. This reduces the amount of log output during normal operation, as INFO-level messages detailing each API call page fetch will no longer be displayed by default. WARNING and ERROR level logs will still be shown. Users needing to debug API calls can temporarily change the verbosity level back to INFO in the script.
- Adds functionality to fetch overall pull request reviews (e.g., approval/changes requested summaries). - Introduces `get_pull_request_reviews()` to call the relevant GitHub API endpoint. - In `main()`: - Fetches these overall reviews. - Filters them to exclude 'DISMISSED' states. - Applies the `--since` argument to filter reviews based on their `submitted_at` timestamp (client-side filtering). - Sorts the filtered reviews chronologically (oldest first). - Prints these overall review summaries under a new "# Overall Review Summaries" header before printing line-specific comments. - Output includes reviewer, submission time, state, review body (if any), and a link to the review. - Handles errors during the fetching of overall reviews separately. - Ensures that the `--since` argument continues to apply independently to line-specific comments based on their `updated_at` timestamp.
Further refines the display of top-level pull request review summaries. Overall reviews that have a state of "COMMENTED" and an empty (or whitespace-only) body are now filtered out and not displayed. This helps to reduce noise from review submissions that don't contain a summary message, while still showing approvals, changes requested, or commented reviews that do have content.
Improves the accuracy and applicability of the suggested next command for fetching subsequent comments. - The script now tracks the latest `submitted_at` timestamp from overall PR reviews, in addition to the latest `updated_at` timestamp from line-specific comments. - The `--since` value in the suggested next command is now based on the globally latest timestamp found across both overall reviews and line comments. - This ensures that a 'next command' suggestion can be provided even if only overall reviews (or only line comments) were found, and that the timestamp used is the most recent activity of either type.
This commit culminates a series of enhancements to create a standalone script for printing GitHub pull request review information. Key changes from the original `get_pr_review_comments.py` and subsequent iterations include: - Renamed script to `scripts/print_github_reviews.py` (from `scripts/gha/get_pr_review_comments_standalone.py`). - Made script standalone by inlining necessary GitHub API interaction functions. - Auto-detection of repository owner/name from git remote. - Auto-detection of PR number from the current git branch if not specified. - Added support for fetching and displaying top-level PR review summaries (e.g., Approve, Comment, Changes Requested) in addition to line comments. - Top-level reviews are filtered for 'DISMISSED' state and by `--since` timestamp. - Empty-bodied 'COMMENTED' top-level reviews are also filtered out. - Enhanced the 'next command' suggestion to use the latest timestamp from either overall reviews or line comments. - Numerous bug fixes and refinements to argument parsing, error handling, and output formatting: - Resolved `UnboundLocalError` for timestamp tracking. - Addressed `argparse` conflict for `--pull_number`. - Removed deprecated nested argument groups. - Standardized error messages to be more concise and suggest `--help`. - Conditional printing of section headers. - Default logging verbosity set to WARNING. - Removed extraneous comments.
- Fixes an `UnboundLocalError` for `latest_overall_review_activity_dt`
by ensuring it and related timestamp-tracking variables are initialized
at the beginning of the `main()` function scope before potential use.
- Removes the `absl-py` dependency from the script:
- Deletes the `absl` import and `logging.set_verbosity()` call.
- All `logging.info()` calls (previously used for API call details)
have been removed to reduce verbosity.
- `logging.error()` calls have been replaced with direct writes to
`sys.stderr`, prefixed with "Error:".
This simplifies dependencies and resolves the runtime error, while ensuring
important error messages are still reported.
- Addresses a persistent `UnboundLocalError` related to `latest_overall_review_activity_dt`. - While the variable was initialized at the top of `main()`, to ensure robust scoping within the loop that processes overall reviews, a temporary variable (`temp_latest_overall_review_dt`) is now used for accumulating the latest timestamp within that specific loop. - The main `latest_overall_review_activity_dt` is then updated from this temporary variable if any overall review timestamps were processed. - This change ensures the variable is always defined before use in the conditional check that was causing the error. (This commit also includes prior changes for removing the absl-py dependency and refactoring logging, which were part of the same active development session.)
…tamps - Corrects the logic for generating the 'next command' suggestion to ensure it appears even if only overall PR reviews (and no line comments) are present. - The script now reliably uses the latest timestamp from either overall review `submitted_at` or line comment `updated_at` fields. - Removed a premature `return` that bypassed the suggestion logic when line comments were empty but overall reviews might have had timestamps. This also includes a pass for final extraneous comment removal.
- Ensures `processed_comments_count` is initialized to 0 at the beginning of the `main()` function scope, alongside other tracking variables like `latest_overall_review_activity_dt` and `latest_line_comment_activity_dt`. - This prevents an `UnboundLocalError` when the script attempts to print the number of processed line comments in scenarios where the line comment processing loop is skipped (e.g., if no line comments are fetched). This commit finalizes fixes for variable initialization and scoping issues.
jonsimantov
commented
Jun 24, 2025
- Removes the unused `TIMEOUT_LONG` constant. - Adds a blank line after imports for PEP 8 compliance. - Changes the output heading for overall review bodies from "### Summary Comment:" to "### Comment:". - Performs a final pass to remove extraneous, obsolete, or iteration-related comments throughout the script, while retaining comments that explain non-obvious logic or important context.
Integration test with FLAKINESS (succeeded after retry)Requested by @jonsimantov on commit 9d37ec0
Add flaky tests to go/fpl-cpp-flake-tracker |
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Description
Testing
Type of Change
Place an
xthe applicable box:Notes
Release Notessection ofrelease_build_files/readme.md.