Skip to content

tox formatting for python files in format.sh#14497

Merged
AutomatedTester merged 6 commits intoSeleniumHQ:trunkfrom
navin772:python-formatting
Oct 18, 2024
Merged

tox formatting for python files in format.sh#14497
AutomatedTester merged 6 commits intoSeleniumHQ:trunkfrom
navin772:python-formatting

Conversation

@navin772
Copy link
Copy Markdown
Member

@navin772 navin772 commented Sep 13, 2024 •
edited by qodo-code-review Bot
Loading

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

  • Added Python formatting capabilities to the format.sh script using tox.
  • Installed tox and configured it to use the linting environment.
  • This change aims to improve code consistency and maintainability by applying standard Python formatting tools like isort, black, flake8, and docformatter.

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 and set TOXENV to linting.
  • Executed tox with configuration from py/tox.ini.
    		•  +7/-0     

    💡 PR-Agent usage: Comment /help "your question" on any pull request to receive relevant information

    @qodo-code-review
    Copy link
    Copy Markdown
    Contributor

    qodo-code-review Bot commented Sep 13, 2024 •
    edited
    Loading

    PR Reviewer Guide 🔍

    (Review updated until commit 7d08453)

    Here are some key observations to aid the review process:

    ⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
    🧪 No relevant tests
    🔒 No security concerns identified
    ⚡ Recommended focus areas for review

    Dependency Management
    The PR installs tox using pip, which may not be the ideal way to manage dependencies in a project using Bazel.

    Environment Variable
    Setting TOXENV as an environment variable might affect other parts of the build process or CI/CD pipeline.

    @qodo-code-review
    Copy link
    Copy Markdown
    Contributor

    qodo-code-review Bot commented Sep 13, 2024 •
    edited
    Loading

    PR Code Suggestions ✨

    Latest suggestions up to 7d08453
    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Score
    Best practice
    Implement a more robust method for installing and running tox in an isolated environment

    Consider using a virtual environment or checking if tox is already installed before
    running 'pip install tox'. This can prevent potential conflicts with system-wide
    packages and ensure a clean, isolated environment for running tox.

    scripts/format.sh [37-39]

    -pip install tox
+if ! command -v tox &> /dev/null; then
+    python -m venv .venv
+    source .venv/bin/activate
+    pip install tox
+fi
 export TOXENV=linting
 tox -c py/tox.ini
    • Apply this suggestion
    Suggestion importance[1-10]: 9

    Why: This suggestion significantly improves the robustness of the script by ensuring that tox is installed in a virtual environment, preventing potential conflicts with system-wide packages. It also checks if tox is already installed, which is a best practice for managing dependencies.

    		9
    Enhancement
    Add error handling for the tox command to improve script robustness

    Add error handling to check if the tox command was successful. This can help
    identify and report any issues that occur during the Python formatting process.

    scripts/format.sh [38-39]

     export 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]: 8

    Why: Adding error handling for the tox command enhances the script's robustness by providing feedback if the command fails. This is a valuable improvement for debugging and maintaining the script.

    		8

    💡 Need additional feedback ? start a PR chat

    Previous suggestions

    Suggestions up to commit d1215ea
    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
    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
    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
    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

    @AutomatedTester
    Copy link
    Copy Markdown
    Member

    AutomatedTester commented Oct 4, 2024

    This seems to not work on the CI. Can you look into it?

    @navin772
    Copy link
    Copy Markdown
    Member Author

    navin772 commented Oct 4, 2024

    I will take a look, from the CI logs it seems like selenium py package failed to build.

    For now, I will convert this to draft.

    @navin772 navin772 marked this pull request as draft October 4, 2024 14:14
    @navin772 navin772 marked this pull request as ready for review October 8, 2024 08:16
    @qodo-code-review
    Copy link
    Copy Markdown
    Contributor

    qodo-code-review Bot commented Oct 8, 2024

    Persistent review updated to latest commit 7d08453

    @navin772
    Copy link
    Copy Markdown
    Member Author

    navin772 commented Oct 8, 2024

    @AutomatedTester I have updated the PR, can you please trigger the CI?

    @navin772
    Copy link
    Copy Markdown
    Member Author

    navin772 commented Oct 8, 2024

    The RBE test failures are unrelated to the changes.

    @AutomatedTester AutomatedTester merged commit 97c29e7 into SeleniumHQ:trunk Oct 18, 2024
    @navin772 navin772 deleted the python-formatting branch October 18, 2024 10:19
    M1troll pushed a commit to M1troll/selenium that referenced this pull request May 14, 2025
    @navin772 @harsha509 @M1troll
    [build] tox formatting for python files in format.sh (SeleniumHQ#14497
    22e9c8b 
    )



---------

Co-authored-by: Sri Harsha <[email protected]>
    @dependabot dependabot Bot mentioned this pull request Sep 18, 2025
    Merged
    @dependabot dependabot Bot mentioned this pull request Oct 1, 2025
    Closed
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

    Reviewers

    @AutomatedTester AutomatedTester AutomatedTester approved these changes

    Assignees

    No one assigned

    Labels

    P-enhancement PR with a new feature Review effort [1-5]: 2

    Projects

    None yet

    Milestone

    No milestone

    Development

    Successfully merging this pull request may close these issues.

    3 participants

    @navin772 @AutomatedTester @harsha509