Skip to content

fixing mypy error from #15693 #15705

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 2 commits into from
May 7, 2025
Merged

Conversation

bandophahita
Copy link
Contributor

@bandophahita bandophahita commented May 6, 2025

User description

🔗 Related Issues

Fixes #15693
And partly addresses #15697

💥 What does this PR do?

Exposes session_id from driver within WebElement for cases when WebElement is used to create WebDriverWait instances.

🔧 Implementation Notes

This seemed like the least intrusive way to avoid attribute error when accessing WebDriverWait.__repr__.

💡 Additional Considerations

🔄 Types of changes

  • Cleanup (formatting, renaming)
  • Bug fix (backwards compatible)
  • New feature (non-breaking change which adds functionality and tests!)
  • Breaking change (fix or feature that would cause existing functionality to change)

PR Type

Bug fix, Enhancement


Description

  • Exposes session_id property on WebElement for better compatibility

  • Updates WebElement.__repr__ to use the new session_id property

  • Adds type annotation to ignored exceptions in WebDriverWait

  • Adds return type annotation to WebDriverWait.__repr__


Changes walkthrough 📝

Relevant files
Enhancement
webelement.py
Add session_id property and update __repr__ in WebElement

py/selenium/webdriver/remote/webelement.py

  • Adds a session_id property to WebElement
  • Updates __repr__ to use the new property
  • +5/-1     
    wait.py
    Add type annotations in WebDriverWait                                       

    py/selenium/webdriver/support/wait.py

  • Adds type annotation to ignored exceptions list
  • Adds return type annotation to __repr__
  • +2/-2     

    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 C-py Python Bindings B-support Issue or PR related to support classes labels May 6, 2025
    @selenium-ci
    Copy link
    Member

    Thank you, @bandophahita for this code suggestion.

    The support packages contain example code that many users find helpful, but they do not necessarily represent
    the best practices for using Selenium, and the Selenium team is not currently merging changes to them.

    We actively encourage people to add the wrapper and helper code that makes sense for them to their own frameworks.
    If you have any questions, please contact us

    Copy link
    Contributor

    qodo-merge-pro bot commented May 6, 2025

    PR Reviewer Guide 🔍

    (Review updated until commit 9126095)

    Here are some key observations to aid the review process:

    🎫 Ticket compliance analysis ❌

    5678 - Not compliant

    Non-compliant requirements:

    • Fix "Error: ConnectFailure (Connection refused)" when instantiating Chrome Driver multiple times

    Requires further human verification:

    • Need to verify if the session_id exposure in WebElement addresses the connection issues with ChromeDriver

    1234 - Not compliant

    Non-compliant requirements:

    • Fix issue where JavaScript in link's href is not triggered on click() in Firefox

    Requires further human verification:

    • Need to verify if the current PR changes have any impact on Firefox click() behavior

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

    Variable Initialization

    The exceptions variable is now type-annotated but not initialized before use. This could cause a NameError if ignored_exceptions is None.

    exceptions: list = list(IGNORED_EXCEPTIONS)
    if ignored_exceptions:
        try:
            exceptions.extend(iter(ignored_exceptions))
        except TypeError:  # ignored_exceptions is not iterable
            exceptions.append(ignored_exceptions)

    Copy link
    Contributor

    qodo-merge-pro bot commented May 6, 2025

    PR Code Suggestions ✨

    Latest suggestions up to 9126095
    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Impact
    Possible issue
    Add error handling

    Add error handling for cases where _parent might not have a session_id
    attribute. This prevents potential AttributeError exceptions when accessing the
    property with incompatible parent objects.

    py/selenium/webdriver/remote/webelement.py [82-84]

     @property
     def session_id(self) -> str:
    -    return self._parent.session_id
    +    if hasattr(self._parent, 'session_id'):
    +        return self._parent.session_id
    +    return ""
    • Apply / Chat
    Suggestion importance[1-10]: 5

    __

    Why: The suggestion proposes adding a hasattr check to prevent an AttributeError if self._parent lacks a session_id. This improves the robustness of the session_id property, particularly for its use in __repr__. However, the absence of session_id on a parent object in Selenium usually indicates a more significant underlying issue, and returning an empty string might mask this. The improvement is moderately beneficial.

    Low
    • Update

    Previous suggestions

    Suggestions up to commit 9126095
    CategorySuggestion                                                                                                                                    Impact
    Possible issue
    Handle missing parent attribute

    Handle the case where _parent might not have a session_id attribute. This could
    happen if the parent object is not properly initialized or is of an unexpected
    type, leading to potential AttributeError.

    py/selenium/webdriver/remote/webelement.py [82-84]

     @property
     def session_id(self) -> str:
    -    return self._parent.session_id
    +    if hasattr(self._parent, 'session_id'):
    +        return self._parent.session_id
    +    return ""
    Suggestion importance[1-10]: 6

    __

    Why: The suggestion correctly points out a potential AttributeError if self._parent does not have a session_id. Adding a hasattr check improves the robustness of the new session_id property, especially as it's used in __repr__.

    Low

    @cgoldberg cgoldberg reopened this May 6, 2025
    @CLAassistant
    Copy link

    CLAassistant commented May 6, 2025

    CLA assistant check
    All committers have signed the CLA.

    @cgoldberg cgoldberg self-requested a review May 7, 2025 00:36
    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.

    @bandophahita @MarcelWilson

    Thanks very much for the contribution... I'm merging it now!

    btw, how do you not get banned for having 2 GH accounts? I'm so confused :)

    @cgoldberg cgoldberg merged commit 2b950b1 into SeleniumHQ:trunk May 7, 2025
    17 checks passed
    @bandophahita
    Copy link
    Contributor Author

    btw, how do you not get banned for having 2 GH accounts? I'm so confused :)

    personal and work accounts.

    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
    Labels
    B-support Issue or PR related to support classes C-py Python Bindings Review effort 2/5
    Projects
    None yet
    Development

    Successfully merging this pull request may close these issues.

    [🐛 Bug]: [py] Mypy error - WebDriverWait.driver has attribute
    5 participants