Skip to content

[py] Add type annotations to bidi network module - #16875

Merged
cgoldberg merged 4 commits into
SeleniumHQ:trunkfrom
mzyndul:py-add-type-hints-bidi-network
Jan 9, 2026
Merged

[py] Add type annotations to bidi network module#16875
cgoldberg merged 4 commits into
SeleniumHQ:trunkfrom
mzyndul:py-add-type-hints-bidi-network

Conversation

@mzyndul

@mzyndul mzyndul commented Jan 9, 2026

Copy link
Copy Markdown
Contributor

User description

🔗 Related Issues

Contributes to ongoing Python type annotation improvements (see #16837, #16821)

💥 What does this PR do?

Adds comprehensive type annotations to the BiDi network module (selenium/webdriver/common/bidi/network.py):

  • NetworkEvent class: Type hints for __init__ and from_json classmethod
  • Network class: Full annotations for all 9 methods, including callbacks, intercepts, and request handlers
  • Request class: Type hints for initialization and all request manipulation methods

🔧 Implementation Notes

  • Uses Callable from collections.abc for callback type hints
  • Uses Any type for complex JSON structures and flexible parameters (cookies, headers, timings)

🔄 Types of changes

  • Cleanup (formatting, renaming)

PR Type

Enhancement


Description

  • Adds comprehensive type annotations to BiDi network module classes

  • Annotates NetworkEvent, Network, and Request classes with type hints

  • Uses Callable, Any, and union types for flexible parameter handling

  • Improves IDE support and enables static type analysis


Diagram Walkthrough

flowchart LR
  A["BiDi Network Module"] --> B["NetworkEvent Class"]
  A --> C["Network Class"]
  A --> D["Request Class"]
  B --> E["Type Hints Added"]
  C --> E
  D --> E
  E --> F["Better IDE Support"]
  E --> G["Static Analysis"]
Loading

File Walkthrough

Relevant files
Enhancement
network.py
Add type hints to network module classes                                 

py/selenium/webdriver/common/bidi/network.py

  • Added imports for Callable from collections.abc and Any from typing
  • Added type annotations to NetworkEvent.__init__ and from_json
    classmethod
  • Added comprehensive type hints to all Network class methods and
    instance variables
  • Added type annotations to Request class __init__ and all manipulation
    methods
  • Used union types (|) for optional parameters and flexible types
  • Used Any type for complex JSON structures and callback parameters
+60/-38 

Add comprehensive type hints to NetworkEvent, Network, and Request classes
in the BiDi network module to improve code quality and enable better IDE
support and static analysis.
@CLAassistant

CLAassistant commented Jan 9, 2026

Copy link
Copy Markdown

CLA assistant check
All committers have signed the CLA.

@selenium-ci selenium-ci added C-py Python Bindings B-devtools Includes everything BiDi or Chrome DevTools related labels Jan 9, 2026
@qodo-code-review

qodo-code-review Bot commented Jan 9, 2026

Copy link
Copy Markdown
Contributor

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

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Meaningful Naming and Self-Documenting Code

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

Status: Passed

Learn more about managing compliance generic rules or creating your own custom rules

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

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Security-First Input Validation and Data Handling

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

Status: Passed

Learn more about managing compliance generic rules or creating your own custom rules

🔴
Generic: Robust Error Handling and Edge Case Management

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

Status:
Generic exception rethrow: The new error handling catches all exceptions and re-raises a generic Exception
stringifying the original error, which loses exception type/stack context and prevents
callers from handling specific failure cases.

Referred Code
try:
    self.conn.execute(command_builder("network.removeIntercept", {"intercept": intercept}))
    self.intercepts.remove(intercept)
except Exception as e:
    raise Exception(f"Exception: {e}")

Learn more about managing compliance generic rules or creating your own custom rules

Generic: Secure Error Handling

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

Status:
Error detail exposure: The new raise Exception(f"Exception: {e}") may expose underlying internal error
details to downstream consumers depending on how this library surfaces exceptions.

Referred Code
try:
    self.conn.execute(command_builder("network.removeIntercept", {"intercept": intercept}))
    self.intercepts.remove(intercept)
except Exception as e:
    raise Exception(f"Exception: {e}")

Learn more about managing compliance generic rules or creating your own custom rules

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

@qodo-code-review

qodo-code-review Bot commented Jan 9, 2026

Copy link
Copy Markdown
Contributor

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
Possible issue
Prevent potential None-access error

In the _on_request method's _callback function, safely access the request
dictionary from event_data.params using .get("request", {}) to prevent potential
runtime errors.

py/selenium/webdriver/common/bidi/network.py [117-151]

 def _on_request(self, event_name: str, callback: Callable[["Request"], Any]) -> int:
     """Set a callback function to subscribe to a network event.
 
     Args:
         event_name: The event to subscribe to.
         callback: The callback function to execute on event.
             Takes Request object as argument.
 
     Returns:
         int: callback id
     """
     event = NetworkEvent(event_name)
 
     def _callback(event_data: NetworkEvent) -> None:
+        request_data = event_data.params.get("request", {})
         request = Request(
             network=self,
-            request_id=event_data.params["request"].get("request", None),
-            body_size=event_data.params["request"].get("bodySize", None),
-            cookies=event_data.params["request"].get("cookies", None),
-            resource_type=event_data.params["request"].get("goog:resourceType", None),
-            headers=event_data.params["request"].get("headers", None),
-            headers_size=event_data.params["request"].get("headersSize", None),
-            timings=event_data.params["request"].get("timings", None),
-            url=event_data.params["request"].get("url", None),
+            request_id=request_data.get("request", None),
+            body_size=request_data.get("bodySize", None),
+            cookies=request_data.get("cookies", None),
+            resource_type=request_data.get("goog:resourceType", None),
+            headers=request_data.get("headers", None),
+            headers_size=request_data.get("headersSize", None),
+            timings=request_data.get("timings", None),
+            url=request_data.get("url", None),
         )
         callback(request)
 
     callback_id: int = self.conn.add_callback(event, _callback)
 
     if event_name in self.callbacks:
         self.callbacks[event_name].append(callback_id)
     else:
         self.callbacks[event_name] = [callback_id]
 
     return callback_id
  • Apply / Chat
Suggestion importance[1-10]: 8

__

Why: The suggestion correctly identifies a potential KeyError or AttributeError if the event data lacks the expected "request" key and provides a robust fix, preventing a likely runtime error.

Medium
Avoid duplicate event_class passes

In the from_json method, avoid passing the event_class argument twice to the
init method by removing it from the dictionary that is unpacked as keyword
arguments.

py/selenium/webdriver/common/bidi/network.py [33-34]

 def from_json(cls, json: dict[str, Any]) -> "NetworkEvent":
-    return cls(event_class=json.get("event_class", ""), **json)
+    data = dict(json)
+    event_class = data.pop("event_class", "")
+    return cls(event_class=event_class, **data)
  • Apply / Chat
Suggestion importance[1-10]: 8

__

Why: The suggestion correctly identifies a potential TypeError due to passing the event_class argument twice and provides a clear fix, preventing a runtime error.

Medium
General
Split dictionary for better type-safety

Split the self.callbacks dictionary into two separate, more specific
dictionaries (self.event_callbacks and self.callback_intercepts) to improve type
safety and code clarity.

py/selenium/webdriver/common/bidi/network.py [56-58]

 self.intercepts: list[str] = []
-self.callbacks: dict[str | int, Any] = {}
+self.event_callbacks: dict[str, list[int]] = {}
+self.callback_intercepts: dict[int, str] = {}
 self.subscriptions: dict[str, list[int]] = {}
  • Apply / Chat
Suggestion importance[1-10]: 7

__

Why: The suggestion correctly identifies that self.callbacks is used for two different purposes, and splitting it would improve type safety and code clarity, which is a significant design improvement.

Medium
  • Update

@cgoldberg cgoldberg left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Thanks.

There is a mypy error:

selenium\webdriver\remote\webdriver.py:1126: error: Argument 1 to "Network" has incompatible type "WebSocketConnection | None"; expected "WebSocketConnection"  [arg-type]

Can you fix that?

Comment thread py/selenium/webdriver/common/bidi/network.py Outdated
Comment thread py/selenium/webdriver/common/bidi/network.py Outdated
Comment thread py/selenium/webdriver/common/bidi/network.py Outdated
@cgoldberg

cgoldberg commented Jan 9, 2026

Copy link
Copy Markdown
Member

I don't think the annotations I mentioned need to be forward references because they are already defined and shouldn't introduce circular imports?

Edit: sorry, it's not a circular import you are trying to get around. I'm just looking at them quickly and don't see why they need to be forward references.. but if they do, you can leave them the way you had them.

  - Remove forward reference quotes from NetworkEvent and Request type hints
  - Add assertion for websocket connection type narrowing in webdriver.py
@mzyndul

mzyndul commented Jan 9, 2026

Copy link
Copy Markdown
Contributor Author

I used

assert self._websocket_connection is None

I think I saw it already in other places,

Question: I could help with type annotation in other places, but I don't want to introduce a big PR. Doing 3-4 files in one PR would be fine?

@cgoldberg

Copy link
Copy Markdown
Member

Doing 3-4 files in one PR would be fine?

yes, that's fine. I don't mind either way, but smaller PR's are easier to review.

@cgoldberg

Copy link
Copy Markdown
Member

weird.. the Docs failed to build with some import errors... I'm not sure why. I'll try to run them again.

@cgoldberg

cgoldberg commented Jan 9, 2026

Copy link
Copy Markdown
Member

It's showing: NameError: name 'NetworkEvent' is not defined.

It's because the annotation is referring to the class it's in. I guess that does need to be a forward reference.

@mzyndul

mzyndul commented Jan 9, 2026

Copy link
Copy Markdown
Contributor Author

I think I know why I need to add now

from __future__ import annotations 

I removed quotes from a few annotations.

On local, I got the same error.

I will push a fix shortly

@mzyndul
mzyndul requested a review from cgoldberg January 9, 2026 19:28

@cgoldberg cgoldberg left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

LGTM! Thanks

@cgoldberg
cgoldberg merged commit c95abca into SeleniumHQ:trunk Jan 9, 2026
24 checks passed
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.

4 participants