Skip to content

Conversation

@iampopovich
Copy link
Contributor

@iampopovich iampopovich commented Nov 4, 2025

User description

🔗 Related Issues

relates to #15697

💥 What does this PR do?

This pull request introduces several improvements and refactorings across multiple modules to enhance type safety, code clarity, and error handling. The most significant changes include updating type annotations to use modern Python syntax, replacing deprecated imports, and adding explicit error checks.

Type annotations and code clarity improvements:

  • Refactored type annotations in common/devtools/pdl.py functions (assignType, createItem, parse, and loads) to use explicit types and modern Python syntax, improving readability and static analysis. [1] [2] [3] [4]
  • Updated variable initialization in py/selenium/webdriver/remote/webdriver.py to use explicit Optional[...] type annotations for better clarity and type checking.
  • Changed the return type of the rp_id property in py/selenium/webdriver/common/virtual_authenticator.py to Optional[str], reflecting that it may be None.
  • Removed type annotations from descriptor assignments in py/selenium/webdriver/safari/options.py to avoid potential issues with descriptor objects.
  • Added necessary imports and type annotations in py/selenium/webdriver/webkitgtk/options.py and updated the type of browser_options for consistency. [1] [2]

Error handling enhancements:

  • Added explicit error checks for browser_name and vendor_prefix in the Chromium WebDriver initialization, raising ValueError if they are not specified.

🔧 Implementation Notes

💡 Additional Considerations

🔄 Types of changes

  • Cleanup (formatting, renaming)

PR Type

Bug fix, Enhancement


Description

  • Fix mypy type warnings across multiple modules

  • Add explicit type annotations to function signatures

  • Validate browser_name and vendor_prefix in ChromiumDriver

  • Update property return types to reflect Optional values

  • Replace deprecated imports with modern Python syntax


Diagram Walkthrough

flowchart LR
  A["Type Annotations"] --> B["Function Signatures"]
  A --> C["Property Return Types"]
  D["Import Updates"] --> E["Modern Python Syntax"]
  F["Validation"] --> G["ChromiumDriver Init"]
  B --> H["Type Safety"]
  C --> H
  E --> H
  G --> H
Loading

File Walkthrough

Relevant files
Enhancement
pdl.py
Add type annotations and modernize imports                             

common/devtools/pdl.py

  • Replaced import collections with from collections import OrderedDict
  • Added from typing import Any import
  • Added explicit type annotations to assignType(), createItem(),
    parse(), and loads() functions
  • Updated all collections.OrderedDict() calls to use imported
    OrderedDict()
+18/-9   
webdriver.py
Add Optional type annotations to BiDi attributes                 

py/selenium/webdriver/remote/webdriver.py

  • Added explicit Optional type annotations to all BiDi-related instance
    variables
  • Updated _websocket_connection, _script, _network, _browser,
    _bidi_session, _browsing_context, _storage, _webextension,
    _permissions, _emulation, _input, and _devtools with proper type hints
+12/-12 
options.py
Add type annotation to browser_options variable                   

py/selenium/webdriver/webkitgtk/options.py

  • Added from typing import Any import
  • Added explicit type annotation dict[str, Any] to browser_options
    variable
+3/-1     
Error handling
webdriver.py
Add parameter validation for ChromiumDriver                           

py/selenium/webdriver/chromium/webdriver.py

  • Added explicit validation checks for browser_name and vendor_prefix
    parameters
  • Raises ValueError if either parameter is None during initialization
+5/-0     
Bug fix
virtual_authenticator.py
Fix rp_id property return type annotation                               

py/selenium/webdriver/common/virtual_authenticator.py

  • Changed rp_id property return type from str to Optional[str]
  • Reflects that the property can return None
+1/-1     
options.py
Remove type annotations from descriptor assignments           

py/selenium/webdriver/safari/options.py

  • Removed type annotations from descriptor assignments for
    automatic_inspection, automatic_profiling, and use_technology_preview
  • Prevents type conflicts with descriptor objects
+3/-3     

selenium/webdriver/common/virtual_authenticator.py:116: error: Incompatible return value type (got "Optional[str]", expected "str") [return-value]
…afety

selenium/webdriver/remote/webdriver.py:1062: error: Incompatible return value type (got "tuple[None, None]", expected "tuple[Any, WebSocketConnection]")  [return-value]
selenium/webdriver/remote/webdriver.py:1095: error: Incompatible return value type (got "None", expected "Script")  [return-value]
selenium/webdriver/remote/webdriver.py:1117: error: Incompatible return value type (got "None", expected "Network")  [return-value]
selenium/webdriver/remote/webdriver.py:1138: error: Incompatible return value type (got "None", expected "Browser")  [return-value]
selenium/webdriver/remote/webdriver.py:1149: error: Incompatible return value type (got "None", expected "Session")  [return-value]
selenium/webdriver/remote/webdriver.py:1170: error: Incompatible return value type (got "None", expected "BrowsingContext")  [return-value]
selenium/webdriver/remote/webdriver.py:1195: error: Incompatible return value type (got "None", expected "Storage")  [return-value]
selenium/webdriver/remote/webdriver.py:1218: error: Incompatible return value type (got "None", expected "Permissions")  [return-value]
selenium/webdriver/remote/webdriver.py:1238: error: Incompatible return value type (got "None", expected "WebExtension")  [return-value]
selenium/webdriver/remote/webdriver.py:1261: error: Incompatible return value type (got "None", expected "Emulation")  [return-value]
selenium/webdriver/remote/webdriver.py:1285: error: Incompatible return value type (got "None", expected "Input")  [return-value]
…initialization

selenium/webdriver/chromium/webdriver.py:61: error: Argument "browser_name" to "ChromiumRemoteConnection" has incompatible type "Optional[str]"; expected "str"  [arg-type]
selenium/webdriver/chromium/webdriver.py:62: error: Argument "vendor_prefix" to "ChromiumRemoteConnection" has incompatible type "Optional[str]"; expected "str"  [arg-type]
selenium/webdriver/webkitgtk/options.py:67: error: Incompatible types in assignment (expression has type "bool", target has type "str") [assignment]
…tributes

selenium/webdriver/safari/options.py:68: error: Incompatible types in assignment (expression has type "_SafariOptionsDescriptor", variable has type "bool")  [assignment]
selenium/webdriver/safari/options.py:71: error: Incompatible types in assignment (expression has type "_SafariOptionsDescriptor", variable has type "bool")  [assignment]
selenium/webdriver/safari/options.py:74: error: Incompatible types in assignment (expression has type "_SafariOptionsDescriptor", variable has type "bool")  [assignment]
@selenium-ci selenium-ci added C-py Python Bindings B-devtools Includes everything BiDi or Chrome DevTools related labels Nov 4, 2025
@iampopovich
Copy link
Contributor Author

@cgoldberg I've edited the most basic type warnings and added type hints where they were obvious. Please check the RDL file; is the type specification there correct?

@qodo-merge-pro
Copy link
Contributor

qodo-merge-pro bot commented Nov 4, 2025

PR Compliance Guide 🔍

Below is a summary of compliance checks for this PR:

Security Compliance
🟢
No security concerns identified No security vulnerabilities detected by AI analysis. Human verification advised for critical code.
Ticket Compliance
🎫 No ticket provided
  • Create ticket/issue
Codebase Duplication Compliance
Codebase context is not defined

Follow the guide to enable codebase context checks.

Custom Compliance
🟢
Generic: Comprehensive Audit Trails

Objective: To create a detailed and reliable record of critical system actions for security analysis
and compliance.

Status: Passed

Generic: Meaningful Naming and Self-Documenting Code

Objective: Ensure all identifiers clearly express their purpose and intent, making code
self-documenting

Status: Passed

Generic: Robust Error Handling and Edge Case Management

Objective: Ensure comprehensive error handling that provides meaningful context and graceful
degradation

Status: Passed

Generic: Secure Error Handling

Objective: To prevent the leakage of sensitive system information through error messages while
providing sufficient detail for internal debugging.

Status: Passed

Generic: Secure Logging Practices

Objective: To ensure logs are useful for debugging and auditing without exposing sensitive
information like PII, PHI, or cardholder data.

Status: Passed

Generic: Security-First Input Validation and Data Handling

Objective: Ensure all data inputs are validated, sanitized, and handled securely to prevent
vulnerabilities

Status: Passed

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 Nov 4, 2025

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
General
Make required arguments explicit in signature

Update the ChromiumDriver.init method signature to make browser_name and
vendor_prefix required arguments, removing the Optional type hint and default
None value, and then remove the now-redundant runtime checks for None.

py/selenium/webdriver/chromium/webdriver.py [31-67]

 def __init__(
     self,
-    browser_name: Optional[str] = None,
-    vendor_prefix: Optional[str] = None,
+    browser_name: str,
+    vendor_prefix: str,
     options: Optional[ChromiumOptions] = None,
     service: Optional[ChromiumService] = None,
     keep_alive: bool = True,
 ) -> None:
     """Create a new WebDriver instance, start the service, and create new ChromiumDriver instance.
 
     Args:
         browser_name: Browser name used when matching capabilities.
         vendor_prefix: Company prefix to apply to vendor-specific WebDriver extension commands.
         options: This takes an instance of ChromiumOptions.
         service: Service object for handling the browser driver if you need to pass extra details.
         keep_alive: Whether to configure ChromiumRemoteConnection to use HTTP keep-alive.
     """
     self.service = service if service else ChromiumService()
     options = options if options else ChromiumOptions()
 
     finder = DriverFinder(self.service, options)
     if finder.get_browser_path():
         options.binary_location = finder.get_browser_path()
         options.browser_version = None
 
     self.service.path = self.service.env_path() or finder.get_driver_path()
     self.service.start()
 
-    if browser_name is None:
-        raise ValueError("browser_name must be specified")
-    if vendor_prefix is None:
-        raise ValueError("vendor_prefix must be specified")
-
     executor = ChromiumRemoteConnection(
         remote_server_addr=self.service.service_url,
         browser_name=browser_name,
         vendor_prefix=vendor_prefix,

[To ensure code accuracy, apply this suggestion manually]

Suggestion importance[1-10]: 7

__

Why: The suggestion correctly identifies a contradiction between the method signature, which marks arguments as optional, and the implementation, which requires them. Aligning the signature makes the API contract clearer and enables static analysis tools to catch errors.

Medium
  • More


@property
def rp_id(self) -> str:
def rp_id(self) -> Optional[str]:
Copy link
Member

Choose a reason for hiding this comment

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

Will _rp_id ever not be a string? If it might be None, we should define the return as: str | None

Copy link
Contributor Author

Choose a reason for hiding this comment

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

_rp_id can be None if rp_id is not in the dictionary and is assigned the default value None.

rp_id = data.get("rpId", None)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If it might be None, we should define the return as: str | None

why not Optional[str] , isn't it equal to str | None ?

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

Labels

B-devtools Includes everything BiDi or Chrome DevTools related C-py Python Bindings Review effort 2/5

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants