Skip to content

Conversation

@kloczek
Copy link

@kloczek kloczek commented May 15, 2024

User description

Description

Filter all python code over pupgrade --py38-plus.

Motivation and Context

Make code cleaner.

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


Description

  • Updated Python code to enhance compatibility with Python 3.8+ by removing outdated syntax and improving string formatting.
  • Removed specific mode arguments in file open functions to use default reading mode, simplifying the code.
  • Transitioned to using format method for string formatting across various scripts and modules.

Changes walkthrough 📝

Relevant files
Enhancement
convert_protocol_to_json.py
Simplify file opening in convert_protocol_to_json.py         

common/devtools/convert_protocol_to_json.py

  • Removed the mode argument from open function to use the default mode.
  • +1/-1     
    pdl.py
    Code modernization in pdl.py                                                         

    common/devtools/pdl.py

  • Removed future import for print_function.
  • Updated print syntax to use format method.
  • +1/-2     
    gen_file.py
    Refactor string formatting and file handling in gen_file.py

    javascript/private/gen_file.py

  • Updated string formatting to use format method.
  • Simplified file opening by removing the mode argument.
  • +22/-22 
    webelement.py
    Update type hints in webelement.py                                             

    py/selenium/webdriver/remote/webelement.py

  • Changed return type hint from List to list for Python 3.9+
    compatibility.
  • +1/-1     
    remote_downloads_tests.py
    Simplify file opening in remote_downloads_tests.py             

    py/test/selenium/webdriver/remote/remote_downloads_tests.py

  • Removed the mode argument from open function to use the default mode.
  • +1/-1     
    pinned_browsers.py
    Refactor string formatting in pinned_browsers.py                 

    scripts/pinned_browsers.py

    • Updated string formatting to use format method.
    +17/-17 
    update_copyright.py
    Simplify file opening in update_copyright.py                         

    scripts/update_copyright.py

    • Simplified file opening by removing the mode argument.
    +1/-1     

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

    Filter all python code over `pupgrade --py38-plus`.
    
    Signed-off-by: Tomasz Kłoczko <[email protected]>
    @CLAassistant
    Copy link

    CLAassistant commented May 15, 2024

    CLA assistant check
    All committers have signed the CLA.

    @qodo-merge-pro qodo-merge-pro bot added the P-enhancement PR with a new feature label May 15, 2024
    @qodo-merge-pro
    Copy link
    Contributor

    PR Description updated to latest commit (363512f)

    @qodo-merge-pro
    Copy link
    Contributor

    PR Review 🔍

    ⏱️ Estimated effort to review [1-5]

    2, because the changes are mostly syntax updates and string formatting improvements, which are straightforward to review. The PR is well-documented and the changes are consistent across multiple files.

    🧪 Relevant tests

    Yes

    ⚡ Possible issues

    Possible Bug: The removal of the file mode "r" in open() calls might not be an issue since it's the default, but it should be confirmed that no binary files are being read, as this could cause unexpected behavior.

    🔒 Security concerns

    No

    @qodo-merge-pro
    Copy link
    Contributor

    qodo-merge-pro bot commented May 16, 2024

    PR Code Suggestions ✨

    CategorySuggestion                                                                                                                                    Score
    Best practice
    Use a context manager for file operations to ensure proper resource management

    Use a context manager to handle file opening and closing to ensure that the file is
    properly closed even if an error occurs during file operations.

    common/devtools/convert_protocol_to_json.py [24-27]

    -input_file = open(file_name)
    -pdl_string = input_file.read()
    -input_file.close()
    +with open(file_name, 'r') as input_file:
    +    pdl_string = input_file.read()
     
    Suggestion importance[1-10]: 8

    Why: This is a valid improvement suggestion as using a context manager for file operations ensures that the file is properly closed even if an error occurs, which is not currently handled in the PR code.

    8
    Enhancement
    ✅ Use f-strings for clearer and more efficient string formatting
    Suggestion Impact:The suggestion to use f-strings for string formatting was implemented in the commit, enhancing readability and performance.

    code diff:

    -        out.write("const {}* const {}[] = {{\n".format(char_type, name))
    +        out.write(f"const {char_type}* const {name}[] = {{\n")

    Replace the string concatenation in the write method with formatted strings for better
    readability and performance.

    javascript/private/gen_file.py [50]

    -out.write("const {}* const {}[] = {{\n".format(char_type, name))
    +out.write(f"const {char_type}* const {name}[] = {{\n")
     
    Suggestion importance[1-10]: 6

    Why: The suggestion to use f-strings enhances readability and performance, aligning with modern Python practices. This is a good suggestion, but not critical.

    6

    @kloczek
    Copy link
    Author

    kloczek commented May 16, 2024

    Signed

    @titusfortner titusfortner modified the milestone: 4.22 Jun 18, 2024
    @titusfortner
    Copy link
    Member

    @kloczek this is not passing our lint tests, can you please update it?

    @VietND96 VietND96 added the C-py Python Bindings label Nov 12, 2024
    @VietND96 VietND96 added the R-awaiting merge PR depends on another PR for merging label Nov 12, 2024
    @codecov
    Copy link

    codecov bot commented Nov 12, 2024

    Codecov Report

    Attention: Patch coverage is 0% with 1 line in your changes missing coverage. Please review.

    Project coverage is 59.32%. Comparing base (57f8398) to head (b92b884).
    Report is 931 commits behind head on trunk.

    Files with missing lines Patch % Lines
    py/selenium/webdriver/remote/webelement.py 0.00% 1 Missing ⚠️
    Additional details and impacted files
    @@            Coverage Diff             @@
    ##            trunk   #13947      +/-   ##
    ==========================================
    + Coverage   58.48%   59.32%   +0.84%     
    ==========================================
      Files          86       91       +5     
      Lines        5270     5883     +613     
      Branches      220      266      +46     
    ==========================================
    + Hits         3082     3490     +408     
    - Misses       1968     2127     +159     
    - Partials      220      266      +46     

    ☔ View full report in Codecov by Sentry.
    📢 Have feedback on the report? Share it here.

    @VietND96 VietND96 changed the title really drop python<=3.7 support [py] really drop python<=3.7 support Nov 13, 2024
    @github-actions github-actions bot added the J-stale Applied to issues that become stale, and eventually closed. label May 14, 2025
    @github-actions github-actions bot closed this May 28, 2025
    @cgoldberg
    Copy link
    Member

    These changes have already essentially been done in #15784

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

    Labels

    C-py Python Bindings J-stale Applied to issues that become stale, and eventually closed. P-enhancement PR with a new feature R-awaiting merge PR depends on another PR for merging Review effort [1-5]: 2

    Projects

    None yet

    Development

    Successfully merging this pull request may close these issues.

    5 participants