Skip to content
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

[py] Format the code using black #13679

Open
wants to merge 1 commit into
base: trunk
Choose a base branch
from
Open

Conversation

shs96c
Copy link
Member

@shs96c shs96c commented Mar 11, 2024

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.bzl
Integrate 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.py
    Add 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.bzl
    Define 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.bzl
    Implement 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.bzl
    Macro 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.bzl
    Integrate Black Lint Tests into Pytest Definition               

    py/private/pytest.bzl

    • Integrated Black lint tests into pytest_test definition.
    +6/-0     
    format.sh
    Add 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.bazel
    Define 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
    WORKSPACE
    Register Python Black Lint Configuration                                 

    WORKSPACE

    • Registered py-black lint configuration in the workspace setup.
    +1/-0     
    BUILD.bazel
    Configure Black Formatting in Python Build                             

    py/BUILD.bazel

  • Added black_config rule instance for Python formatting configuration.
  • +10/-3   
    Dependencies
    2 files
    requirements.txt
    Add Black to Python Requirements                                                 

    py/requirements.txt

    • Added black to the list of Python requirements.
    +1/-0     
    requirements_lock.txt
    Lock 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

    Copy link

    PR Description updated to latest commit (9a3dae5)

    Copy link

    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.

    Copy link

    codiumai-pr-agent-pro bot commented Mar 11, 2024

    PR Code Suggestions

    CategorySuggestions                                                                                                                                                       
    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 💎 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.

    @titusfortner
    Copy link
    Member

    Should also replace this:

    selenium/Rakefile

    Lines 652 to 655 in d65e38e

    desc 'Update Python Syntax'
    task :lint do
    sh 'tox -c py/tox.ini -e linting'
    end

    Copy link
    Member

    @p0deje p0deje left a comment

    Choose a reason for hiding this comment

    The reason will be displayed to describe this comment to others. Learn more.

    Would this replace the current linting for Python via Tox (isort+black+flake8+docformatter)? It is only of the things that we couldn't migrate to Bazel on CI -

    lint:
    name: Lint
    needs: build
    runs-on: ubuntu-20.04
    steps:
    - name: Checkout source tree
    uses: actions/checkout@v4
    - name: Set up Python 3.8
    uses: actions/setup-python@v4
    with:
    python-version: 3.8
    - name: Install dependencies
    run: |
    python -m pip install --upgrade pip
    pip install tox==4.6.4
    - name: Test with tox
    run: tox -c py/tox.ini
    env:
    # If this fails, it will exit. Local work should be using `tox -e linting` prior to committing.
    # the linting-ci recipe exists solely for CI and will not attempt to rewrite any files in-place etc.
    TOXENV: linting-ci

    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Projects
    None yet
    Development

    Successfully merging this pull request may close these issues.

    None yet

    3 participants