Type
enhancement, documentation
Description
- Integrated Black code formatting into the build and test infrastructure using Bazel.
- Added Black configuration, binary execution script, and testing rules.
- Configured Black formatting for Python code with specific line length.
- Registered
py-black lint configuration for workspace setup.
- Included Black in Python requirements and locked dependencies.
Changes walkthrough
| Relevant files |
|---|
| Enhancement | 8 files
defs.bzlIntegrate Black Formatting into Bazel Build Definitions
py/defs.bzl
Imported and utilized py_with_lint_macro.bzl for py_binary and
py_library definitions.
Added black_config to the list of available macros.
|
+5/-0 |
black-bin.pyAdd Black Binary Execution Script
py/private/black-bin.py
Added a new Python script to execute Black as a binary with patched
main function.
Included license information and basic execution setup.
|
+25/-0 |
black_config.bzlDefine Black Configuration Rule
py/private/black_config.bzl
Defined BlackConfigInfo provider with fields for black binary, line
length, and Python versions.
Implemented black_config rule to provide Black configuration.
|
+31/-0 |
black_test.bzlImplement Black Testing Rule
py/private/black_test.bzl
Created a black_test rule implementation for running Black as tests on
specified sources.
Utilized BlackConfigInfo for test configuration.
|
+63/-0 |
py_with_lint_macro.bzlMacro Definitions for Python Linting with Black
py/private/py_with_lint_macro.bzl
Defined macros for adding lint tests (black_test) to Python targets.
Integrated Black linting into py_binary, py_library, and py_test
definitions.
|
+34/-0 |
pytest.bzlIntegrate Black Lint Tests into Pytest Definition
py/private/pytest.bzl
- Integrated Black lint tests into
pytest_test definition.
|
+6/-0 |
format.shAdd Python Formatting with Black to Format Script
scripts/format.sh
Added a section to format Python code using Black with specific line
length.
|
+5/-0 |
BUILD.bazelDefine Black Binary Target and Alias
py/private/BUILD.bazel
Defined black-bin as a py_binary target.
Made black an alias to black-bin.
|
+20/-1 |
|
| Configuration changes | 2 files
WORKSPACERegister Python Black Lint Configuration
WORKSPACE
- Registered
py-black lint configuration in the workspace setup.
|
+1/-0 |
BUILD.bazelConfigure Black Formatting in Python Build
py/BUILD.bazel
Added black_config rule instance for Python formatting configuration.
|
+10/-3 |
|
| Dependencies | 2 files
requirements.txtAdd Black to Python Requirements
py/requirements.txt
- Added
black to the list of Python requirements.
|
+1/-0 |
requirements_lock.txtLock Black and Its Dependencies
py/requirements_lock.txt
- Included
black and its dependencies in the locked requirements.
|
+45/-1 |
|
✨ PR-Agent usage:
Comment /help on the PR to get a list of all available PR-Agent tools and their descriptions
PR Description updated to latest commit (https://github.com/SeleniumHQ/selenium/commit/9a3dae5d2d28569fe62870a883b830b06b204b3f)
PR Review
| ⏱️ Estimated effort to review [1-5] |
2, because the PR primarily involves integrating and configuring Black for Python code formatting. The changes are straightforward and mostly involve adding new files and configurations rather than modifying existing logic.
|
| 🧪 Relevant tests |
Yes, the PR includes a black_test rule in py/private/black_test.bzl which seems to be designed to ensure that Python code adheres to the formatting standards set by Black.
|
| 🔍 Possible issues |
Possible Oversight: The black_test implementation in py/private/black_test.bzl uses --workers 1 which might slow down the formatting check for a large number of files. Consider removing this or making the number of workers configurable. |
|
Consistency Concern: The line length is set to 120 in scripts/format.sh and py/BUILD.bazel but the default for Black is 88. Ensure this is intentionally set and consistent across all configurations. |
| 🔒 Security concerns |
No
|
✨ Review tool usage guide:
Overview:
The review tool scans the PR code changes, and generates a PR review. The tool can be triggered automatically every time a new PR is opened, or can be invoked manually by commenting on any PR.
When commenting, to edit configurations related to the review tool (pr_reviewer section), use the following template:
/review --pr_reviewer.some_config1=... --pr_reviewer.some_config2=...
With a configuration file, use the following template:
[pr_reviewer]
some_config1=...
some_config2=...
Utilizing extra instructions
The review tool can be configured with extra instructions, which can be used to guide the model to a feedback tailored to the needs of your project.
Be specific, clear, and concise in the instructions. With extra instructions, you are the prompter. Specify the relevant sub-tool, and the relevant aspects of the PR that you want to emphasize.
Examples for extra instructions:
[pr_reviewer] # /review #
extra_instructions="""
In the 'possible issues' section, emphasize the following:
- Does the code logic cover relevant edge cases?
- Is the code logic clear and easy to understand?
- Is the code logic efficient?
...
"""
Use triple quotes to write multi-line instructions. Use bullet points to make the instructions more readable.
|
How to enable\disable automation
- When you first install PR-Agent app, the default mode for the
review tool is:
pr_commands = ["/review", ...]
meaning the review tool will run automatically on every PR, with the default configuration.
Edit this field to enable/disable the tool, or to change the used configurations
|
Auto-labels
The review tool can auto-generate two specific types of labels for a PR:
- a
possible security issue label, that detects possible security issues (enable_review_labels_security flag)
- a
Review effort [1-5]: x label, where x is the estimated effort to review the PR (enable_review_labels_effort flag)
|
Extra sub-tools
The review tool provides a collection of possible feedbacks about a PR.
It is recommended to review the possible options, and choose the ones relevant for your use case.
Some of the feature that are disabled by default are quite useful, and should be considered for enabling. For example:
require_score_review, require_soc2_ticket, and more.
|
Auto-approve PRs
By invoking:
/review auto_approve
The tool will automatically approve the PR, and add a comment with the approval.
To ensure safety, the auto-approval feature is disabled by default. To enable auto-approval, you need to actively set in a pre-defined configuration file the following:
[pr_reviewer]
enable_auto_approval = true
(this specific flag cannot be set with a command line argument, only in the configuration file, committed to the repository)
You can also enable auto-approval only if the PR meets certain requirements, such as that the estimated_review_effort is equal or below a certain threshold, by adjusting the flag:
[pr_reviewer]
maximal_review_effort = 5
|
More PR-Agent commands
To invoke the PR-Agent, add a comment using one of the following commands:
- /review: Request a review of your Pull Request.
- /describe: Update the PR title and description based on the contents of the PR.
- /improve [--extended]: Suggest code improvements. Extended mode provides a higher quality feedback.
- /ask <QUESTION>: Ask a question about the PR.
- /update_changelog: Update the changelog based on the PR's contents.
- /add_docs 💎: Generate docstring for new components introduced in the PR.
- /generate_labels 💎: Generate labels for the PR based on the PR's contents.
- /analyze 💎: Automatically analyzes the PR, and presents changes walkthrough for each component.
See the tools guide for more details.
To list the possible configuration parameters, add a /config comment.
|
See the review usage page for a comprehensive guide on using this tool.
PR Code Suggestions
| Category | Suggestions |
| Enhancement |
Use list comprehension for adding items to a list.
Consider using a list comprehension to accumulate ctx.files.srcs into all_inputs instead
of using extend method in a separate line. This enhances readability and efficiency.
py/private/black_test.bzl [20]
-all_inputs.extend(ctx.files.srcs)
+all_inputs += [src for src in ctx.files.srcs]
Add validation for Python version strings.
For the python_version attribute in black_config rule, consider adding a validation
function to ensure that each version string follows a specific format or belongs to a
predefined set of supported versions.
py/private/black_config.bzl [29]
-"python_version": attr.string_list(),
+"python_version": attr.string_list(validator = _validate_python_version),
Dynamically retrieve --line-length from configuration.
To ensure that the Python formatting command respects the project's configurations,
consider dynamically retrieving the --line-length value from the //py:black-config instead
of hardcoding it in the script.
scripts/format.sh [22]
-bazel run //py/private:black -- --line-length 120 "$(pwd)/py"
+LINE_LENGTH=$(bazel query 'attr("line_length", //py:black-config, //py/private:black_config.bzl)' --output=build | grep "line_length" | cut -d " " -f 2)
+bazel run //py/private:black -- --line-length $LINE_LENGTH "$(pwd)/py"
| | | Possible issue |
Remove redundant loop for adding Python versions.
Since config.python_version is checked for truthiness before adding --target-version,
consider removing the redundant loop that adds -t for each version, as it seems to be a
mistake or an unnecessary duplication.
py/private/black_test.bzl [12-13]
-for version in config.python_version:
- args.add_all(["-t", version])
+# Removed the loop to avoid unnecessary duplication
| |
| Best practice |
Use f-strings for string concatenation.
Instead of manually concatenating strings with the "+" operator, use f-strings for better
readability and performance when constructing the name for black_test.
py/private/py_with_lint_macro.bzl [9]
-name = "%s-black" % name,
+name = f"{name}-black",
Pin black to a specific major version range to avoid potential breaking changes.
Consider pinning the black version to a specific major version range in
py/requirements.txt to avoid potential breaking changes when new major versions are
released. For example, instead of black==24.2.0, you could use black~=24.2.
py/requirements_lock.txt [21]
-black==24.2.0
+black~=24.2
Add a py_test target for the black-bin.py script.
Consider adding a py_test target for the black-bin.py script to ensure its functionality
is verified through automated tests. This can help catch issues early and maintain code
quality.
py/private/BUILD.bazel [19-28]
py_binary(
name = "black-bin",
srcs = [
# If the source is called `black` you can't then import black into black
"black-bin.py",
],
legacy_create_init = False,
deps = [
requirement("black"),
],
)
+py_test(
+ name = "black-bin_test",
+ srcs = ["black-bin_test.py"],
+ deps = [
+ ":black-bin",
+ # other dependencies
+ ],
+)
+
Run black against the entire codebase after adding it to requirements.txt.
After adding black to the requirements.txt, ensure to run it against the entire codebase
and review the changes. This helps in maintaining a consistent code style across the
project.
py/requirements.txt [3]
+# After adding black, run it against the codebase to ensure consistency.
black==24.2.0
| | | Maintainability |
Add a comment explaining the choice of line_length in the black_config rule.
For the black_config rule, consider adding a comment explaining the choice of line_length
= 120. This helps maintainers understand the rationale behind this configuration choice,
especially if it deviates from the default or project-wide style guidelines.
py/BUILD.bazel [11-14]
+# We use a line length of 120 for better readability and to accommodate longer expressions without line breaks.
black_config(
name = "black-config",
line_length = 120,
visibility = ["//visibility:public"],
)
Ensure py-black linter is properly configured and documented.
Ensure that the py-black linter defined in the lint_setup is properly configured and
documented. This includes verifying that the black-config target exists and is correctly
set up, and adding documentation on how and when to run the linter.
WORKSPACE [23]
+# Ensure to document the usage of py-black linter in the project's README or CONTRIBUTING guidelines.
lint_setup({
"java-spotbugs": "//java:spotbugs-config",
"py-black": "//py:black-config",
"rust-rustfmt": "//rust:enable-rustfmt",
})
| | | |
✨ Improve tool usage guide:
Overview:
The improve tool scans the PR code changes, and automatically generates suggestions for improving the PR code. The tool can be triggered automatically every time a new PR is opened, or can be invoked manually by commenting on a PR.
When commenting, to edit configurations related to the improve tool (pr_code_suggestions section), use the following template:
/improve --pr_code_suggestions.some_config1=... --pr_code_suggestions.some_config2=...
With a configuration file, use the following template:
[pr_code_suggestions]
some_config1=...
some_config2=...
Enabling\disabling automation
When you first install the app, the default mode for the improve tool is:
pr_commands = ["/improve --pr_code_suggestions.summarize=true", ...]
meaning the improve tool will run automatically on every PR, with summarization enabled. Delete this line to disable the tool from running automatically.
|
Utilizing extra instructions
Extra instructions are very important for the improve tool, since they enable to guide the model to suggestions that are more relevant to the specific needs of the project.
Be specific, clear, and concise in the instructions. With extra instructions, you are the prompter. Specify relevant aspects that you want the model to focus on.
Examples for extra instructions:
[pr_code_suggestions] # /improve #
extra_instructions="""
Emphasize the following aspects:
- Does the code logic cover relevant edge cases?
- Is the code logic clear and easy to understand?
- Is the code logic efficient?
...
"""
Use triple quotes to write multi-line instructions. Use bullet points to make the instructions more readable.
|
A note on code suggestions quality
- While the current AI for code is getting better and better (GPT-4), it's not flawless. Not all the suggestions will be perfect, and a user should not accept all of them automatically.
- Suggestions are not meant to be simplistic. Instead, they aim to give deep feedback and raise questions, ideas and thoughts to the user, who can then use his judgment, experience, and understanding of the code base.
- Recommended to use the 'extra_instructions' field to guide the model to suggestions that are more relevant to the specific needs of the project, or use the custom suggestions :gem: tool
- With large PRs, best quality will be obtained by using 'improve --extended' mode.
|
More PR-Agent commands
To invoke the PR-Agent, add a comment using one of the following commands:
- /review: Request a review of your Pull Request.
- /describe: Update the PR title and description based on the contents of the PR.
- /improve [--extended]: Suggest code improvements. Extended mode provides a higher quality feedback.
- /ask <QUESTION>: Ask a question about the PR.
- /update_changelog: Update the changelog based on the PR's contents.
- /add_docs 💎: Generate docstring for new components introduced in the PR.
- /generate_labels 💎: Generate labels for the PR based on the PR's contents.
- /analyze 💎: Automatically analyzes the PR, and presents changes walkthrough for each component.
See the tools guide for more details.
To list the possible configuration parameters, add a /config comment.
|
See the improve usage page for a more comprehensive guide on using this tool.
Should also replace this:
https://github.com/SeleniumHQ/selenium/blob/d65e38e34fc6ac29b7c2c62cc0b924d7f8762e6d/Rakefile#L652-L655
SeleniumHQ
![qodo-code-review[bot] avatar](https://avatars.githubusercontent.com/in/484649?v=4 "Published by a pub.dev verified publisher"){kind=small}
