[py] Fix : Mypy type annotation errors - 2 by ShauryaDusht · Pull Request #15848 · SeleniumHQ/selenium

ShauryaDusht · 2025-06-03T08:28:24Z

User description

🔧 What does this PR do?

This PR fixes several type annotation errors reported by Mypy in the following files:

selenium/webdriver/common/bidi/browser.py - changes reverted to avoid conflict from another PR fixed errors in browser.py for 15697 #15847
selenium/webdriver/common/bidi/browsing_context.py
selenium/webdriver/common/action_chains.py - recently added
selenium/webdriver/common/utils.py - recently added

💡 Additional Notes

This is part of ongoing efforts to improve type annotations across the Selenium codebase.
No behavior changes are introduced; only type hints are corrected.
This is my second PR on the issue.

🔄 Types of changes

Code cleanup (type annotation fixes, no functional changes)

PR Type

Bug fix, Enhancement

qodo-code-review · 2025-06-03T08:28:44Z

PR Reviewer Guide 🔍

Here are some key observations to aid the review process:

⏱️ Estimated effort to review: 2 🔵🔵⚪⚪⚪
🧪 No relevant tests
🔒 No security concerns identified
⚡ Recommended focus areas for review Variable Shadowing The variable `event_obj` is created to replace the shadowing of the parameter `event` that was happening in the original code. This is a good fix, but should be verified that it doesn't affect any other code that might be relying on the original variable name. event_obj = BrowsingContextEvent(event_name) self.conn.remove_callback(event_obj, callback_id) Default Values The PR adds default empty string values for string types and 0 for numeric types. Verify that these defaults are appropriate for the application context and won't cause unexpected behavior when API responses are missing fields. context=str(json.get("context", "")), navigation=json.get("navigation"), timestamp=int(json.get("timestamp", 0)), url=str(json.get("url", "")),

qodo-code-review · 2025-06-03T08:29:29Z

PR Code Suggestions ✨

Latest suggestions up to 5eec4b8

Category	Suggestion	Impact
Incremental ^[*]	✅ ~~Prevent potential KeyError~~ Suggestion Impact: The commit fixed the potential KeyError issue by restructuring the code to safely access self.subscriptions[event_name]. The implementation uses a different approach than suggested but addresses the same issue by ensuring proper checks before accessing the dictionary. code diff: event_obj = BrowsingContextEvent(event_name) self.conn.remove_callback(event_obj, callback_id) - if event_name in self.subscriptions and callback_id in self.subscriptions[event_name]: - self.subscriptions[event_name].remove(callback_id) - if len(self.subscriptions[event_name]) == 0: - params = {"events": [event_name]} - session = Session(self.conn) - self.conn.execute(session.unsubscribe(params)) - del self.subscriptions[event_name] + if event_name in self.subscriptions: + callbacks = self.subscriptions[event_name] + if callback_id in callbacks: + callbacks.remove(callback_id) + if not callbacks: + params = {"events": [event_name]} + session = Session(self.conn) + self.conn.execute(session.unsubscribe(params)) + del self.subscriptions[event_name] The code attempts to access `self.subscriptions[event_name]` after checking if `event_name` is in `self.subscriptions`, but then immediately accesses it again on the next line without checking. This could cause a KeyError if `event_name` is not in `self.subscriptions`. py/selenium/webdriver/common/bidi/browsing_context.py [716-718] + self.conn.remove_callback(event_obj, callback_id) + if event_name in self.subscriptions and callback_id in self.subscriptions[event_name]: + self.subscriptions[event_name].remove(callback_id) ++ if len(self.subscriptions[event_name]) == 0: ++ params = {"events": [event_name]} ++ session = Session(self.conn) ++ self.conn.execute(session.unsubscribe(**params)) ++ del self.subscriptions[event_name] `[To ensure code accuracy, apply this suggestion manually]` Suggestion importance[1-10]: 8 __ Why: This identifies a real bug where `self.subscriptions[event_name]` is accessed on line 719 without checking if `event_name` exists in `self.subscriptions`, which could cause a `KeyError`. The improved code correctly moves the length check inside the conditional block.	Medium
General	✅ ~~Add warning for unexpected types~~ Suggestion Impact: The commit implemented the exact suggestion by replacing the list comprehension with a for loop that adds a warning when encountering non-dictionary child types in the browsing context. code diff: +import warnings from typing import Any, Callable, Optional, Union from selenium.webdriver.common.bidi.common import command_builder @@ -110,7 +111,12 @@ children = None raw_children = json.get("children") if raw_children is not None and isinstance(raw_children, list): - children = [BrowsingContextInfo.from_json(child) for child in raw_children if isinstance(child, dict)] + children = [] + for child in raw_children: + if isinstance(child, dict): + children.append(BrowsingContextInfo.from_json(child)) + else: + warnings.warn(f"Unexpected child type in browsing context: {type(child)}") The current implementation may silently ignore non-dictionary items in the children list without any warning. Consider adding a warning or log message when encountering unexpected child types to help with debugging. py/selenium/webdriver/common/bidi/browsing_context.py [111-113] raw_children = json.get("children") if raw_children is not None and isinstance(raw_children, list): - children = [BrowsingContextInfo.from_json(child) for child in raw_children if isinstance(child, dict)] + children = [] + for child in raw_children: + if isinstance(child, dict): + children.append(BrowsingContextInfo.from_json(child)) + else: + import warnings + warnings.warn(f"Unexpected child type in browsing context: {type(child)}") `[Suggestion processed]` Suggestion importance[1-10]: 4 __ Why: The suggestion adds debugging warnings for unexpected child types, which could be helpful but is not critical. The PR already improved safety by adding `isinstance(child, dict)` filtering.	Low
Update

Previous suggestions

✅ Suggestions up to commit e75da48

Category	Suggestion	Impact
Possible issue	✅ ~~Prevent ValueError on removal~~ Suggestion Impact: The commit directly implemented the suggestion by adding a check to verify if event_name exists in subscriptions and if callback_id exists in subscriptions[event_name] before attempting to remove it code diff: - self.subscriptions[event_name].remove(callback_id) + if event_name in self.subscriptions and callback_id in self.subscriptions[event_name]: + self.subscriptions[event_name].remove(callback_id) Check if the callback_id exists in the subscriptions before attempting to remove it. The current implementation will raise a ValueError if the callback_id is not in the list, which could happen if it was already removed or never added. py/selenium/webdriver/common/bidi/browsing_context.py [716] def remove_event_handler(self, event: str, callback_id: int) -> None: """Remove an event handler from the browsing context. Parameters: ---------- event: The event to unsubscribe from. callback_id: The callback id to remove. """ try: event_name = self.EVENTS[event] except KeyError: raise Exception(f"Event {event} not found") event_obj = BrowsingContextEvent(event_name) self.conn.remove_callback(event_obj, callback_id) - self.subscriptions[event_name].remove(callback_id) + if event_name in self.subscriptions and callback_id in self.subscriptions[event_name]: + self.subscriptions[event_name].remove(callback_id) Suggestion importance[1-10]: 7 __ Why: Valid suggestion that prevents potential `ValueError` when attempting to remove a non-existent `callback_id` from the subscriptions list, improving error handling robustness.	Medium
Possible issue	✅ ~~Validate dictionary type before processing~~ Suggestion Impact: The commit directly implemented the suggestion by adding the 'isinstance(child, dict)' check in the list comprehension, exactly as suggested code diff: - children = [BrowsingContextInfo.from_json(child) for child in raw_children] + children = [BrowsingContextInfo.from_json(child) for child in raw_children if isinstance(child, dict)] Add type checking for each child element before calling from_json to prevent runtime errors. The current code assumes each child is a dictionary, but if any non-dictionary value is present in the list, it will cause a runtime error. py/selenium/webdriver/common/bidi/browsing_context.py [111-113] raw_children = json.get("children") if raw_children is not None and isinstance(raw_children, list): - children = [BrowsingContextInfo.from_json(child) for child in raw_children] + children = [BrowsingContextInfo.from_json(child) for child in raw_children if isinstance(child, dict)] `[Suggestion processed]` Suggestion importance[1-10]: 6 __ Why: Valid suggestion to prevent runtime errors when non-dictionary values are in the children list, though this is likely an edge case in normal usage.	Low

ShauryaDusht · 2025-06-03T11:48:19Z

Hi @cgoldberg, please review my PR (I have added a few more files in this PR)

navin772 · 2025-06-03T12:23:33Z

@ShauryaDusht another PR (#15847) is resolving the type errors for browser.py file, can you pick another file to avoid conflict?

ShauryaDusht · 2025-06-03T12:28:13Z

@ShauryaDusht another PR (#15847) is resolving the type errors for browser.py file, can you pick another file to avoid conflict?

Will remove browser.py changes and update the PR with another file. Thanks!

navin772 · 2025-06-04T06:08:13Z

@ShauryaDusht I don't think silently defaulting to a value (like an empty string) when the value is not found is a good idea. This will introduce uncertainty and does not conform to the BiDi spec.
For example, the changes made for UserPromptOpenedParams, default_value is set to an empty string if not found but the spec says:
the defaultValue field set to default value if default value is not null or omitted otherwise
which means it should either be omitted from the payload or should be None.

So, we should do something like this:

		default_value = json.get("defaultValue")
        if default_value is not None and not isinstance(default_value, str):
           raise ValueError("defaultValue must be a string if provided")

Same with other classes and params.

ShauryaDusht · 2025-06-10T12:04:59Z

Hi @navin772
Please review my changes

py/selenium/webdriver/common/utils.py

py/selenium/webdriver/common/bidi/browsing_context.py

navin772

@ShauryaDusht I have made some changes as per the BiDi spec.

LGTM!

ShauryaDusht · 2025-06-11T08:41:37Z

Thanks for the changes and review

[py] Fix : Mypy type annotation errors

e75da48

selenium-ci added C-py Python Bindings B-devtools Includes everything BiDi or Chrome DevTools related labels Jun 3, 2025

qodo-code-review bot added the Review effort 2/5 label Jun 3, 2025

Minor fixes

5eec4b8

ShauryaDusht changed the title ~~[py] Fix : Mypy type annotation errors~~ [py] Fix : Mypy type annotation errors - 2 Jun 3, 2025

ShauryaDusht and others added 4 commits June 3, 2025 18:00

Reverted changes in py\selenium\webdriver\common\bidi\browser.py

c1db637

[py] Fix: two more files

7e6e39d

Remove browser.py from PR

4dbf5b0

Merge branch 'trunk' into fix-mypy-type-errors-2

66d8051

ShauryaDusht added 3 commits June 4, 2025 12:31

suggested changes

196ec5a

made suggested changes

638008b

suggested change in each class and param

8bbfee0

Merge branch 'trunk' into fix-mypy-type-errors-2

107ddea

navin772 requested changes Jun 10, 2025

View reviewed changes

py/selenium/webdriver/common/utils.py Show resolved Hide resolved

py/selenium/webdriver/common/bidi/browsing_context.py Outdated Show resolved Hide resolved

py/selenium/webdriver/common/bidi/browsing_context.py Show resolved Hide resolved

Minor fixes

1c48bb0

ShauryaDusht requested a review from navin772 June 11, 2025 06:26

navin772 added 2 commits June 11, 2025 13:29

make changes as per bidi spec

aadee6d

Merge branch 'trunk' into fix-mypy-type-errors-2

c779fd2

navin772 approved these changes Jun 11, 2025

View reviewed changes

navin772 merged commit 6e0c875 into SeleniumHQ:trunk Jun 11, 2025
28 of 29 checks passed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[py] Fix : Mypy type annotation errors - 2#15848

[py] Fix : Mypy type annotation errors - 2#15848
navin772 merged 13 commits intoSeleniumHQ:trunkfrom
ShauryaDusht:fix-mypy-type-errors-2

ShauryaDusht commented Jun 3, 2025 •

edited

Loading

Uh oh!

qodo-code-review bot commented Jun 3, 2025

Uh oh!

qodo-code-review bot commented Jun 3, 2025 •

edited

Loading

Uh oh!

ShauryaDusht commented Jun 3, 2025

Uh oh!

navin772 commented Jun 3, 2025

Uh oh!

ShauryaDusht commented Jun 3, 2025

Uh oh!

navin772 commented Jun 4, 2025

Uh oh!

ShauryaDusht commented Jun 10, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

navin772 left a comment

Uh oh!

ShauryaDusht commented Jun 11, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

Uh oh!

Conversation

ShauryaDusht commented Jun 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

User description

🔧 What does this PR do?

💡 Additional Notes

🔄 Types of changes

PR Type

Uh oh!

qodo-code-review bot commented Jun 3, 2025

PR Reviewer Guide 🔍

Uh oh!

qodo-code-review bot commented Jun 3, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

PR Code Suggestions ✨

Previous suggestions

Uh oh!

ShauryaDusht commented Jun 3, 2025

Uh oh!

navin772 commented Jun 3, 2025

Uh oh!

ShauryaDusht commented Jun 3, 2025

Uh oh!

navin772 commented Jun 4, 2025

Uh oh!

ShauryaDusht commented Jun 10, 2025

Uh oh!

Uh oh!

Uh oh!

Uh oh!

navin772 left a comment

Choose a reason for hiding this comment

Uh oh!

ShauryaDusht commented Jun 11, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

4 participants

ShauryaDusht commented Jun 3, 2025 •

edited

Loading

qodo-code-review bot commented Jun 3, 2025 •

edited

Loading