Skip to content

[py] Fix: Mypy type annotation errors - 5 #15908

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
Closed
wants to merge 1 commit into from
Closed

[py] Fix: Mypy type annotation errors - 5 #15908

wants to merge 1 commit into from

Conversation

@ShauryaDusht
Copy link
Contributor

@ShauryaDusht ShauryaDusht commented Jun 18, 2025 •
edited by cgoldberg
Loading

User description

🔗 Related Issues

#15697

💥 What does this PR do?

This PR fixes type annotation issues in the following files:

  • py/selenium/webdriver/support/event_firing_webdriver.py
  • py/selenium/webdriver/webkitgtk/service.py
  • py/selenium/webdriver/wpewebkit/service.py

🔧 Implementation Notes

Uses getattr/setattr for safer attribute manipulation and fixes type annotations to match actual shutil.which() return type.

💡 Additional Considerations

🔄 Types of changes

  • Bug fix (backwards compatible)

PR Type

Bug fix

Description

• Fix MyPy type annotation errors in WebDriver support and service modules
• Update shutil.which() return type annotations to Optional[str]
• Improve attribute manipulation safety using getattr/setattr
• Fix log file handling in WebKit service constructors

Changes walkthrough 📝

Relevant files
Bug fix
event_firing_webdriver.py
Improve attribute manipulation safety                                       

py/selenium/webdriver/support/event_firing_webdriver.py

• Replace direct attribute assignment with safer getattr/setattr
pattern
• Store original _wrap_value attribute before modification

 +2/-1     
service.py
Fix type annotations and log handling                                       

py/selenium/webdriver/webkitgtk/service.py

• Change DEFAULT_EXECUTABLE_PATH type annotation to Optional[str]
•
Update constructor parameter type to match shutil.which() return type

• Fix log file handling with proper variable naming

 +8/-6     
service.py
Fix executable path type annotation                                           

py/selenium/webdriver/wpewebkit/service.py

• Update constructor parameter type annotation to Optional[str]
•
Match shutil.which() return type for executable path

 +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.
    •

    @selenium-ci selenium-ci added C-py Python Bindings B-support Issue or PR related to support classes labels Jun 18, 2025
    @selenium-ci
    Copy link
    Member

    selenium-ci commented Jun 18, 2025

    Thank you, @ShauryaDusht 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

    @qodo-merge-pro
    Copy link
    Contributor

    qodo-merge-pro bot commented Jun 18, 2025

    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

    Resource Leak

    File handles opened for log_file and output_file are never closed, potentially causing resource leaks. The opened files should be properly managed or closed after use.

     
        log_file = open(log_path, "wb")
if log_output:
    output_file = open(log_output, "wb")
    Missing Cleanup

    The original _wrap_value attribute is stored but never restored when the EventFiringWebDriver is destroyed or cleaned up, potentially leaving the wrapped driver in a modified state.

     
    self._original_wrap_value = getattr(self._driver, "_wrap_value", None)
setattr(self._driver, "_wrap_value", self._wrap_value)

    @qodo-merge-pro
    Copy link
    Contributor

    qodo-merge-pro bot commented Jun 18, 2025

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Impact
    General
    Add proper file handle cleanup

    The opened file handles are not properly closed, which can lead to resource
    leaks. Consider using context managers or ensure proper cleanup in a destructor
    or cleanup method.

    py/selenium/webdriver/webkitgtk/service.py [52-58]

     log_file = None
 output_file = None
 if log_path is not None:
     warnings.warn("log_path is deprecated, use log_output instead", DeprecationWarning, stacklevel=2)
     log_file = open(log_path, "wb")
 if log_output:
     output_file = open(log_output, "wb")
+# Store file handles for proper cleanup
+self._log_file = log_file
+self._output_file = output_file
    • Apply / Chat
    Suggestion importance[1-10]: 7

    __

    Why: The suggestion correctly identifies a potential resource leak. If both log_path (deprecated) and log_output are provided, two files are opened. However, the logic log_output=log_file or output_file only passes one of these file handles to the superclass for cleanup, leaving the other one open.

    		Medium
    • More

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

    Reviewers

    No reviews

    Assignees

    No one assigned

    Labels

    B-support Issue or PR related to support classes C-py Python Bindings Review effort 3/5

    Projects

    None yet

    Milestone

    No milestone

    Development

    Successfully merging this pull request may close these issues.

    2 participants

    @ShauryaDusht @selenium-ci