Skip to content

Conversation

@ShauryaDusht
Copy link
Contributor

@ShauryaDusht ShauryaDusht commented Oct 21, 2025

User description

🔗 Related Issues

#15697

💥 What does this PR do?

Fixes mypy type errors in Python webdriver modules:

  • selenium/webdriver/common/virtual_authenticator.py: Made rp_id Optional[str] to match actual usage
  • selenium/webdriver/remote/errorhandler.py: Added type guards before .get() calls on potentially None values
  • selenium/webdriver/webkitgtk/service.py: Made executable_path Optional[str] to match default value
  • selenium/webdriver/wpewebkit/service.py: Used separate variable for file objects to avoid type conflicts

🔧 Implementation Notes

Type annotation fixes only.....no runtime behavior changes.

💡 Additional Considerations

No tests or docs needed.

🔄 Types of changes

  • Cleanup (formatting, renaming)
  • Bug fix (backwards compatible)

PR Type

Bug fix, Tests


Description

  • Fixed mypy type errors in Python webdriver modules

  • Made rp_id parameter Optional[str] in virtual authenticator

  • Added type guard for dict check before .get() call in error handler

  • Made executable_path parameter Optional[str] in service modules

  • Refactored file object handling to avoid type conflicts


Diagram Walkthrough

flowchart LR
  A["Type Annotation Fixes"] --> B["virtual_authenticator.py"]
  A --> C["errorhandler.py"]
  A --> D["webkitgtk/service.py"]
  A --> E["wpewebkit/service.py"]
  B --> B1["rp_id: Optional[str]"]
  C --> C1["Add isinstance dict check"]
  D --> D1["executable_path: Optional[str]<br/>Refactor file handling"]
  E --> E1["executable_path: Optional[str]"]
Loading

File Walkthrough

Relevant files
Bug fix
virtual_authenticator.py
Made rp_id parameter optional                                                       

py/selenium/webdriver/common/virtual_authenticator.py

  • Changed rp_id parameter type from str to Optional[str] in __init__
    method
  • Updated rp_id property return type to Optional[str] to match actual
    usage
+2/-2     
errorhandler.py
Add type guard for dict check                                                       

py/selenium/webdriver/remote/errorhandler.py

  • Added isinstance(message, dict) type guard before calling .get()
    method
  • Prevents potential AttributeError when message is not a dictionary
+2/-1     
service.py
Fix type annotations and refactor file handling                   

py/selenium/webdriver/webkitgtk/service.py

  • Changed DEFAULT_EXECUTABLE_PATH type from str to Optional[str] to
    match shutil.which() return type
  • Updated executable_path parameter type to Optional[str] in __init__
    method
  • Refactored file object handling by introducing separate log_file
    variable
  • Changed logic to use elif for clearer control flow between log_path
    and log_output
  • Added IO type import for proper type annotation
+10/-6   
service.py
Make executable_path parameter optional                                   

py/selenium/webdriver/wpewebkit/service.py

  • Changed executable_path parameter type from str to Optional[str] in
    __init__ method
  • Aligns with default value from DEFAULT_EXECUTABLE_PATH
+1/-1     

@selenium-ci selenium-ci added the C-py Python Bindings label Oct 21, 2025
@qodo-merge-pro
Copy link
Contributor

qodo-merge-pro bot commented Oct 21, 2025

PR Compliance Guide 🔍

Below is a summary of compliance checks for this PR:

Security Compliance
Resource leak (files)

Description: Opened log files assigned to log_file are not closed, which can leak file descriptors and
exhaust resources; consider context managers or ensuring closure in service teardown.
service.py [55-65]

Referred Code
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")
elif log_output is not None:
    log_file = open(log_output, "wb")

super().__init__(
    executable_path=executable_path,
    port=port,
    log_output=log_file,
    env=env,
Ticket Compliance
🎫 No ticket provided
- [ ] Create ticket/issue <!-- /create_ticket --create_ticket=true -->

</details></td></tr>
Codebase Duplication Compliance
Codebase context is not defined

Follow the guide to enable codebase context checks.

Custom Compliance
No custom compliance provided

Follow the guide to enable custom compliance check.

  • Update
Compliance status legend 🟢 - Fully Compliant
🟡 - Partial Compliant
🔴 - Not Compliant
⚪ - Requires Further Human Verification
🏷️ - Compliance label

@qodo-merge-pro
Copy link
Contributor

qodo-merge-pro bot commented Oct 21, 2025

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
General
Prioritize new parameter over deprecated one
Suggestion Impact:The commit reversed the if/elif checks to prioritize log_output over log_path, matching the suggestion, and additionally wrapped the logic in a try/except to safely close the file on errors.

code diff:

+        try:
+            if log_output is not None:
+                log_file = open(log_output, "wb")
+            elif log_path is not None:
+                warnings.warn("log_path is deprecated, use log_output instead", DeprecationWarning, stacklevel=2)
+                log_file = open(log_path, "wb")
 

Prioritize the log_output parameter over the deprecated log_path by reversing
the order of the if/elif checks to align with standard deprecation practices.

py/selenium/webdriver/webkitgtk/service.py [55-59]

-if log_path is not None:
+if log_output is not None:
+    log_file = open(log_output, "wb")
+elif log_path is not None:
     warnings.warn("log_path is deprecated, use log_output instead", DeprecationWarning, stacklevel=2)
     log_file = open(log_path, "wb")
-elif log_output is not None:
-    log_file = open(log_output, "wb")

[Suggestion processed]

Suggestion importance[1-10]: 7

__

Why: The suggestion correctly identifies a logical flaw in the PR where a deprecated parameter (log_path) is prioritized over its replacement (log_output), and proposes the correct fix.

Medium
Learned
best practice
Close opened log files safely
Suggestion Impact:The commit wrapped the log file opening and superclass initialization in a try/except block and closed the file if an exception occurred, implementing safe resource handling.

code diff:

+        try:
+            if log_output is not None:
+                log_file = open(log_output, "wb")
+            elif log_path is not None:
+                warnings.warn("log_path is deprecated, use log_output instead", DeprecationWarning, stacklevel=2)
+                log_file = open(log_path, "wb")
 
-        super().__init__(
-            executable_path=executable_path,
-            port=port,
-            log_output=log_file,
-            env=env,
-            **kwargs,
-        )
+            super().__init__(
+                executable_path=executable_path,
+                port=port,
+                log_output=log_file,
+                env=env,
+                **kwargs,
+            )
+        except Exception:
+            if log_file is not None:
+                try:
+                    log_file.close()
+                except Exception:
+                    pass
+            raise

Ensure that any file opened for logs is properly closed on all code paths by
delegating closing responsibility or using a context manager/cleanup hook.

py/selenium/webdriver/webkitgtk/service.py [53-67]

 log_file: Optional[IO[bytes]] = None
+try:
+    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")
+    elif log_output is not None:
+        log_file = open(log_output, "wb")
 
-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")
-elif log_output is not None:
-    log_file = open(log_output, "wb")
+    super().__init__(
+        executable_path=executable_path,
+        port=port,
+        log_output=log_file,
+        env=env,
+        **kwargs,
+    )
+except Exception:
+    if log_file is not None:
+        try:
+            log_file.close()
+        except Exception:
+            pass
+    raise
 
-super().__init__(
-    executable_path=executable_path,
-    port=port,
-    log_output=log_file,
-    env=env,
-    **kwargs,
-)
-

[Suggestion processed]

Suggestion importance[1-10]: 6

__

Why:
Relevant best practice - Enforce deterministic and safe resource handling by ensuring opened files are closed via context managers or try/finally.

Low
  • Update

Copy link
Member

@Delta456 Delta456 left a comment

Choose a reason for hiding this comment

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

Please resolve merge conflicts first

@cgoldberg cgoldberg changed the title [py] : Fix mypy type errors in webdriver modules [py] Fix mypy type errors in webdriver modules Oct 21, 2025
@cgoldberg
Copy link
Member

I don't think there is anything in this PR we want to merge.

@cgoldberg cgoldberg closed this Oct 21, 2025
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