-
Notifications
You must be signed in to change notification settings - Fork 121
Add reviewbot.py #4129
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Open
shreyas-goenka
wants to merge
9
commits into
main
Choose a base branch
from
reviewbot
base: main
Could not load branches
Branch not found: {{ refName }}
Loading
Could not load tags
Nothing to show
Loading
Are you sure you want to change the base?
Some commits from the old base branch may be removed from the timeline,
and old review comments may become outdated.
Open
Add reviewbot.py #4129
Changes from all commits
Commits
Show all changes
9 commits
Select commit
Hold shift + click to select a range
a358e30
Add reviewbot.py
shreyas-goenka 7f46261
=
shreyas-goenka bc0d58e
commit publish_review.py
shreyas-goenka 07c7853
only write the review once
shreyas-goenka 4b94e15
another iteration
shreyas-goenka 1c0a461
always publish review
shreyas-goenka 5f791d9
format
shreyas-goenka d1cdb06
fix typo
shreyas-goenka e180b82
add review and edit option
shreyas-goenka File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
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
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,62 @@ | ||
| # PR Review Guidelines | ||
|
|
||
| ## Overview | ||
| You are reviewing a Pull Request for the Databricks CLI. Please conduct a thorough code review following these guidelines. | ||
|
|
||
| ## Review Checklist | ||
|
|
||
| ### Code Quality | ||
| - [ ] Code follows the project's style guide and conventions | ||
| - [ ] Functions and variables have clear, descriptive names | ||
|
|
||
| ### Testing | ||
| - [ ] Appropriate test coverage for new functionality | ||
shreyas-goenka marked this conversation as resolved.
Show resolved
Hide resolved
|
||
| - [ ] Tests are clear and test the right behavior | ||
| - [ ] Golden files in acceptance tests are valid and make sense. Any error messages are readable and understandable as a new user. | ||
|
|
||
| ### Edge Case Analysis | ||
| - [ ] Identify edge cases for the changed code (e.g. empty inputs, nil values, boundary conditions, error paths) | ||
| - [ ] Verify edge cases have acceptance test coverage | ||
|
|
||
| ### Documentation | ||
| - [ ] Code includes necessary comments for complex logic | ||
| - [ ] Public APIs are documented | ||
| - [ ] README or other docs updated if needed | ||
|
|
||
| ### Dependencies | ||
| - [ ] Any new dependencies introduced (e.g. Github Actions, go.mod libraries) are from a trusted source, i.e. either a popular public project or trusted entity like Databricks. | ||
| - [ ] All 3rd party Github actions should be pinned to a specific commit commit | ||
|
|
||
| ### PR metadata | ||
| - [ ] The PR title should be clean and descriptive. | ||
| - [ ] The PR description should provide adequate details about the changes in the PR. | ||
|
|
||
|
|
||
| ## Review Process | ||
| 1. **Understand the Context**: Read the PR description, linked issues and the code / changes. | ||
| 2. **Review code coverage**: For all changed files / code, ensure that the code being added is adequately tested. Strong preference should be given to end-to-end acceptance tests in acceptance/ over unit tests, unless the change being tested is a well defined and clean function / library. | ||
| 3. **Identify gaps in testing / detect bugs (important)** Identify and construct edge cases that are not covered in existing tests. Contruct and run relevant acceptance tests (preferred) and unit tests to ensure these edge cases have a good behavior. Report when missing coverage is detected. | ||
| 4. **Ensure adequate documentation**: Non trivial functions / code changes should have the appropriate amount of doc strings, so that's a new visitor to the code can quickly understand what's happening. This is even more important for abstract changes that use reflection for example. | ||
|
|
||
| ## Output Format | ||
|
|
||
| Please provide your review in the following format: | ||
|
|
||
| ### Summary | ||
| Brief overview of the changes and overall assessment. | ||
|
|
||
| ### Positive Aspects | ||
| What was done well. | ||
|
|
||
| ### Issues Found | ||
| List any bugs, errors, or serious concerns. Any edge cases that fail or should be covered. | ||
|
|
||
| ### Suggestions | ||
| Recommendations for improvement (not blocking). | ||
|
|
||
| ### Questions | ||
| Any clarifications needed. | ||
|
|
||
| ## Command reference: | ||
| 1. You can run and generate golden files for local acceptance tests by running: go test -timeout 300s -run ^TestAccept/$1$ github.com/databricks/cli/acceptance -update <path-to-test-from-acceptance/> (e.g. selftest) | ||
| 2. You can run and generate golden files for cloud acceptance tests by running: deco env run -i -n azure-prod-ucws -- go test -timeout 600s -run ^TestAccept/$1$ github.com/databricks/cli/acceptance -update <path-to-test-from-acceptance/> | ||
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
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -0,0 +1,18 @@ | ||
| #!/usr/bin/env python3 | ||
| import json | ||
| import sys | ||
|
|
||
|
|
||
| def main(): | ||
| if len(sys.argv) < 2: | ||
| print("Usage: publish_review.py '<json>'") | ||
| sys.exit(1) | ||
|
|
||
| review = json.loads(sys.argv[1]) | ||
| with open("/tmp/reviewbot_output.json", "w") as f: | ||
| json.dump(review, f, indent=2) | ||
| print("Review saved. Awaiting user confirmation to publish.") | ||
|
|
||
|
|
||
| if __name__ == "__main__": | ||
| main() |
Oops, something went wrong.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Which are? Should it have a link?
Uh oh!
There was an error while loading. Please reload this page.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I figured claude would figure out the style guide by osmosis. We can add a dedicated style guide later one. Things like acceptance tests could use style consolidation.