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

tox formatting for python files in format.sh #14497

Open
wants to merge 3 commits into
base: trunk
Choose a base branch
from

Conversation

navin772
Copy link
Contributor

@navin772 navin772 commented Sep 13, 2024

User description

Thanks for contributing to Selenium!
A PR well described will help maintainers to quickly review and merge it

Before submitting your PR, please check our contributing guidelines.
Avoid large PRs, help reviewers by making them as simple and short as possible.

Description

It would be contributor friendly if we can have python formatting in ./scripts/format.sh.
Currently due to lack of formatting support from rules_python, bazel targets were not configurable (Added it as a TODO)

Types of changes

  • Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to change)

Checklist

  • I have read the contributing document.
  • My change requires a change to the documentation.
  • I have updated the documentation accordingly.
  • I have added tests to cover my changes.
  • All new and existing tests passed.

PR Type

enhancement, configuration changes


Description

  • Introduced Python formatting in the format.sh script using tox.
  • Configured tox to apply isort, black, flake8, and docformatter for Python files.
  • Added a TODO note for using Bazel targets when rules_python supports formatting.

Changes walkthrough 📝

Relevant files
Enhancement
format.sh
Add Python formatting using tox in format.sh script           

scripts/format.sh

  • Added a new section for Python formatting.
  • Installed tox for Python linting and formatting.
  • Configured tox to use isort, black, flake8, and docformatter.
  • Added a TODO note for future Bazel target support.
  • +7/-0     

    💡 PR-Agent usage:
    Comment /help on the PR to get a list of all available PR-Agent tools and their descriptions

    Copy link

    PR Reviewer Guide 🔍

    ⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
    🧪 No relevant tests
    🔒 No security concerns identified
    ⚡ Key issues to review

    Dependency Management
    The script installs tox using pip, which may not be the ideal way to manage dependencies in a project of this scale. Consider using a more robust dependency management approach.

    Error Handling
    The script doesn't include error handling for the tox installation or execution. This could lead to silent failures or incomplete formatting.

    Copy link

    PR Code Suggestions ✨

    CategorySuggestion                                                                                                                                    Score
    Error handling
    Add error handling for the formatting command

    Add error handling to check if the tox command was successful. This will help catch
    and report any issues during the formatting process.

    scripts/format.sh [38-39]

     TOXENV=linting
    -tox -c py/tox.ini
    +if ! tox -c py/tox.ini; then
    +  echo "Error: Python formatting failed" >&2
    +  exit 1
    +fi
     
    • Apply this suggestion
    Suggestion importance[1-10]: 9

    Why: Adding error handling for the tox command is crucial for catching and reporting issues during the formatting process, improving the robustness and reliability of the script.

    9
    Best practice
    Use a virtual environment for managing Python dependencies

    Consider using a virtual environment or a requirements file to manage dependencies
    instead of directly installing tox with pip. This approach is more reproducible and
    avoids potential conflicts with system-wide packages.

    scripts/format.sh [37]

    -pip install tox
    +python -m venv venv
    +source venv/bin/activate
    +pip install -r requirements.txt  # Include tox in requirements.txt
     
    • Apply this suggestion
    Suggestion importance[1-10]: 8

    Why: Using a virtual environment or a requirements file is a best practice for managing dependencies, ensuring reproducibility, and avoiding conflicts with system-wide packages. This suggestion significantly improves the maintainability and reliability of the script.

    8
    Optimization
    Check if the required tool is already installed before installation

    Consider adding a check to verify if tox is already installed before attempting to
    install it. This can save time and avoid unnecessary installations.

    scripts/format.sh [37]

    -pip install tox
    +if ! command -v tox &> /dev/null; then
    +  pip install tox
    +fi
     
    • Apply this suggestion
    Suggestion importance[1-10]: 7

    Why: Checking if tox is already installed before attempting installation is a useful optimization that can save time and resources, although it is not as critical as the other suggestions.

    7

    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.

    2 participants