Skip to content

[py] Fix: Mypy type annotation errors in remote/webdriver.py #15900

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

Merged
merged 4 commits into from
Jun 17, 2025

Conversation

ShauryaDusht
Copy link
Contributor

@ShauryaDusht ShauryaDusht commented Jun 15, 2025

User description

🔗 Related Issues

Resolves some mypy type issues for #15697

💥 What does this PR do?

Fixes mypy type annotation errors in py/selenium/webdriver/remote/webdriver.py.

🔧 Implementation Notes

Improved type annotations and error handling

🔄 Types of changes

  • Cleanup (formatting, renaming)
  • Bug fix (type hints)

PR Type

Other


Description

• Fix mypy type annotation errors in remote webdriver
• Add proper type hints for class attributes and variables
• Improve error handling with type casting and null checks
• Update function parameters with specific type annotations


Changes walkthrough 📝

Relevant files
Bug fix
webdriver.py
Fix mypy type annotations and error handling                         

py/selenium/webdriver/remote/webdriver.py

• Added missing type imports (Dict, Type, cast)
• Fixed type
annotations for class attributes and method parameters
• Added null
checks and type casting for command executor
• Improved variable
naming and type safety

+25/-14 

Need help?
  • Type /help how to ... in the comments thread for any questions about Qodo Merge usage.
  • Check out the documentation for more information.
  • @selenium-ci selenium-ci added the C-py Python Bindings label Jun 15, 2025
    Copy link
    Contributor

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

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

    Logic Error

    The code assigns remote_server_addr but doesn't use it when the condition fails. The else branch assigns None to nothing, which is a no-op. This could cause undefined variable errors.

    if hasattr(command_executor, "client_config") and command_executor.client_config:
        remote_server_addr = command_executor.client_config.remote_server_addr
    else:
        None
    Missing Assignment

    The variable response is used without being assigned when the executor call is moved. This will cause a NameError when the response variable is referenced later.

    executor = cast(RemoteConnection, self.command_executor)
    response = executor.execute(driver_command, params)
    Type Safety

    The type annotation allows both Dict[str, Any] and Any for options variable, which defeats the purpose of type safety. Should be more specific about the expected type.

    options: Union[Dict[str, Any], Any] = {}
    if print_options:

    Copy link
    Contributor

    qodo-merge-pro bot commented Jun 15, 2025

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Impact
    Possible issue
    Fix incomplete variable assignment
    Suggestion Impact:The suggestion was directly implemented - the problematic "None" statement in the else branch was replaced with "remote_server_addr = command_executor" exactly as suggested

    code diff:

         else:
    -        None
    +        remote_server_addr = command_executor

    The else branch assigns None to nothing, which is a no-op. This should assign
    None to remote_server_addr or handle the case where command_executor doesn't
    have the expected attributes.

    py/selenium/webdriver/remote/webdriver.py [126-129]

     if hasattr(command_executor, "client_config") and command_executor.client_config:
         remote_server_addr = command_executor.client_config.remote_server_addr
     else:
    -    None
    +    remote_server_addr = command_executor

    [Suggestion processed]

    Suggestion importance[1-10]: 10

    __

    Why: The suggestion correctly identifies a critical bug. The else: None statement is a no-op, which would cause a NameError for remote_server_addr on line 132 if the if condition is false. The proposed fix correctly restores the original behavior for the else case.

    High
    • Update

    Copy link
    Member

    @navin772 navin772 left a comment

    Choose a reason for hiding this comment

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

    LGTM!

    @navin772 navin772 requested a review from cgoldberg June 17, 2025 12:27
    @cgoldberg
    Copy link
    Contributor

    re-running CI.. it was running for over 3 hours so I killed it.

    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.

    LGTM.

    (the 2 CI failures are from Java tests, unrelated to this PR)

    @cgoldberg cgoldberg merged commit b73da5e into SeleniumHQ:trunk Jun 17, 2025
    28 of 31 checks passed
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    Projects
    None yet
    Development

    Successfully merging this pull request may close these issues.

    4 participants