Skip to content

Conversation

@cgoldberg
Copy link
Member

@cgoldberg cgoldberg commented May 6, 2025

User description

💥 What does this PR do?

This PR adjusts the linting configurations in tox.ini to include files in the /scripts directory. This also fixes all linting errors discovered in those files.

🔧 Implementation Notes

I ran into 2 weird issues:

  • black formatter was unable to discover files in subdirectories if you include a parent directory, so I had to invoke black on ../scripts separately
  • flake8 and autoflake are unable to process files in a parent directory, so I didn't change their settings to include ../scripts

🔄 Types of changes

  • Test infrastructure

PR Type

Enhancement


Description

  • Expanded linting to include /scripts directory

  • Fixed linting errors in all /scripts Python files

  • Updated tox.ini to run formatters and linters on /scripts

  • Improved code formatting and consistency in scripts


Changes walkthrough 📝

Relevant files
Configuration changes
tox.ini
Expand linting configuration to cover `/scripts` directory

py/tox.ini

  • Updated linting commands to include ../scripts directory
  • Added comments explaining tool limitations for parent directories
  • Ensured black, isort, and docformatter run on /scripts
  • +18/-4   
    Formatting
    pinned_browsers.py
    Fix lint errors and reformat `pinned_browsers.py`               

    scripts/pinned_browsers.py

  • Reformatted code for PEP8 compliance and readability
  • Improved argument parsing and HTTP request formatting
  • Applied consistent string quoting and line breaks
  • Removed unused variable and fixed minor style issues
  • +43/-18 
    selenium_manager.py
    Fix lint errors and reformat `selenium_manager.py`             

    scripts/selenium_manager.py

  • Reformatted code for PEP8 compliance and readability
  • Improved HTTP request formatting and string handling
  • Applied consistent quoting and line breaks
  • Updated dictionary key access for consistency
  • +24/-17 
    update_cdp.py
    Fix lint errors and reformat `update_cdp.py`                         

    scripts/update_cdp.py

  • Reformatted code for PEP8 compliance and readability
  • Improved argument parsing and HTTP request formatting
  • Applied consistent string quoting and line breaks
  • Enhanced function formatting and multi-line expressions
  • +50/-18 
    update_copyright.py
    Fix lint errors and reformat `update_copyright.py`             

    scripts/update_copyright.py

  • Reformatted code for PEP8 compliance and readability
  • Improved class and function formatting
  • Applied consistent string quoting and line breaks
  • Enhanced logic for copyright notice handling
  • +33/-17 

    Need help?
  • Type /help how to ... in the comments thread for any questions about Qodo Merge usage.
  • Check out the documentation for more information.
  • @qodo-merge-pro
    Copy link
    Contributor

    qodo-merge-pro bot commented May 6, 2025

    PR Reviewer Guide 🔍

    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

    Undefined Variable

    The variable 'content' is used before being defined in the selenium_manager function. This could cause a NameError when the function is called.

    fetch_and_save(
        f"https://raw.githubusercontent.com/chromium/chromium/{chrome_milestone['version']}/third_party/blink/public/devtools_protocol/browser_protocol.pdl",

    @qodo-merge-pro
    Copy link
    Contributor

    qodo-merge-pro bot commented May 6, 2025

    PR Code Suggestions ✨

    No code suggestions found for the PR.

    @cgoldberg cgoldberg requested a review from titusfortner May 6, 2025 01:38
    @selenium-ci selenium-ci added C-py Python Bindings B-build Includes scripting, bazel and CI integrations labels May 6, 2025
    @titusfortner
    Copy link
    Member

    Thanks!

    I'm going to move this to draft and assign myself, because I need to run all 4 of those scripts to make sure they don't do something weird before we can merge this.

    @titusfortner titusfortner self-assigned this May 7, 2025
    @titusfortner titusfortner marked this pull request as draft May 7, 2025 00:56
    @p0deje
    Copy link
    Member

    p0deje commented May 16, 2025

    I guess we can close this in favor of #15746

    @cgoldberg
    Copy link
    Member Author

    I just merged #15746 ... closing this one.

    @cgoldberg cgoldberg closed this May 17, 2025
    @cgoldberg cgoldberg deleted the py-lint-scripts-dir branch May 21, 2025 19:49
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

    Labels

    B-build Includes scripting, bazel and CI integrations C-py Python Bindings Review effort 2/5

    Projects

    None yet

    Development

    Successfully merging this pull request may close these issues.

    4 participants