Skip to content

Fix type error for attribute in remote_connection.py #15810

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 1 commit into from
May 29, 2025
Merged

Fix type error for attribute in remote_connection.py #15810

merged 1 commit into from
May 29, 2025

Conversation

Bradltr95
Copy link
Contributor

@Bradltr95 Bradltr95 commented May 28, 2025 •
edited
Loading

User description

💥 What does this PR do?

This update resolves type inconsistencies related to the browser_name variable in webdriver/remote/remote_connection.py. Previously, browser_name was initialized with None, while other parts of the codebase expected it to always be a str, leading to mypy errors such as:
error: Argument "browser_name" to "ChromiumRemoteConnection" has incompatible type "str | None"; expected "str"

Fix

The fix involves updating the type annotation of browser_name to Optional[str], aligning with its usage and ensuring compatibility with static type checking. This change eliminates related type errors and moves the codebase closer to full mypy compliance.

PR Type

Bug fix

Description

  • Fixes type annotation for browser_name in remote_connection.py

  • Changes browser_name to Optional[str] to resolve Mypy errors

  • Addresses part of the static typing issues in Python bindings

Changes walkthrough 📝

Relevant files
Bug fix
remote_connection.py
Correct type annotation for browser_name attribute             

py/selenium/webdriver/remote/remote_connection.py

  • Updates browser_name attribute type to Optional[str]
  • Resolves type error flagged by Mypy static analysis
    		• +1/-1     
    Need help?
  • Type /help how to ... in the comments thread for any questions about Qodo Merge usage.
  • Check out the documentation for more information.
    •

    @CLAassistant
    Copy link

    CLAassistant commented May 28, 2025 •
    edited
    Loading

    CLA assistant check
    All committers have signed the CLA.

    @selenium-ci selenium-ci added the C-py Python Bindings label May 28, 2025
    @qodo-merge-pro Qodo Merge Pro
    Copy link
    Contributor

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

    🎫 Ticket compliance analysis 🔶

    15697 - Partially compliant

    Compliant requirements:

    • Partially fixes type annotation errors in Python bindings by addressing the browser_name type in remote_connection.py

    Non-compliant requirements:

    • Does not fix all type errors identified by Mypy (only addresses one specific case)
    • Does not update CI job to fail when Mypy encounters errors

    Requires further human verification:

    • Verify that this change actually resolves the specific Mypy errors related to browser_name
    • Confirm how many of the 120 reported errors this change addresses

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

    Missing Import

    The code adds a type annotation using Optional but doesn't import it from typing module. This will cause a NameError at runtime when type checking is enabled.

     
    browser_name: Optional[str] = None
# Keep backward compatibility for AppiumConnection - https://github.com/SeleniumHQ/selenium/issues/14694

    @qodo-merge-pro Qodo Merge Pro
    Copy link
    Contributor

    PR Code Suggestions ✨

    No code suggestions found for the PR.

    @Bradltr95 Bradltr95 mentioned this pull request May 28, 2025
    Open
    @cgoldberg cgoldberg self-requested a review May 28, 2025 23:07
    cgoldberg
    cgoldberg reviewed May 28, 2025
    View reviewed changes
    Copy link
    Contributor

    @cgoldberg cgoldberg left a comment

    Choose a reason for hiding this comment

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

    thanks... Can you please sign the CLA? #15810 (comment)

    btw, you can ignore the comment from AI bot complaining about the missing import.. I have no idea what it's talking about :)

    @Bradltr95
    Copy link
    Contributor Author

    thanks... Can you please sign the CLA? #15810 (comment)

    btw, you can ignore the comment from AI bot complaining about the missing import.. I have no idea what it's talking about :)

    Sounds good, do I need to manually ignore it or do you just mean for reference?

    Aso, I signed the CLA as requested :)

    @cgoldberg
    Copy link
    Contributor

    thanks

    do I need to manually ignore it or do you just mean for reference?

    no, you don't need to do anything. I just meant don't be alarmed that it left an irrelevant comment on your PR.

    @Bradltr95
    Copy link
    Contributor Author

    Bradltr95 commented May 29, 2025 •
    edited
    Loading

    @cgoldberg How can I address the CI - RBE tests error? Should I increase the timeout to see if it helps? Or is there something else I should do to get it passing?

    @cgoldberg
    Copy link
    Contributor

    cgoldberg commented May 29, 2025 •
    edited
    Loading

    @Bradltr95 neither of those failures look related to your PR. Unfortunately we have a few flaky tests. I just kicked off CI again to see if it passes, but don't worry about it.. your PR looks fine. I'll keep my eye on the CI runs and merge your change in a little while.

    p.s. if you want to try to make those tests more robust, that would be great.. but do it in a separate PR

    @cgoldberg
    Copy link
    Contributor

    All tests passed.

    Thanks for your first Selenium contribution!

    @cgoldberg cgoldberg merged commit 9f938ae into SeleniumHQ:trunk May 29, 2025
    21 of 23 checks passed
    @Bradltr95 Bradltr95 deleted the remote_conection_type_errors branch May 29, 2025 14:24
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Reviewers

    @cgoldberg cgoldberg cgoldberg left review comments

    Assignees
    No one assigned
    Labels
    C-py Python Bindings Review effort 1/5
    Projects
    None yet
    Milestone
    No milestone
    Development

    Successfully merging this pull request may close these issues.
    4 participants
    @Bradltr95 @CLAassistant @cgoldberg @selenium-ci