Py typing

[duxtec]

User description

Motivation and Context

This change improves code maintainability and enhances static type checking by adding type hints to:

• Helps detect potential issues earlier with tools like MyPy and Pyright.
• Improves code readability for contributors and maintainers.
• Aligns with modern Python best practices for type safety.

Files:

• selenium.webdriver.remote.webdriver.py
• selenium.webdriver.remote.webelement.py
• selenium.webdriver.remote.remote_connection.py
• selenium.webdriver.remote.script_key.py
• selenium.webdriver.common.options.py

Types of changes

• [x] Bug fix (non-breaking change which fixes an issue)
• [ ] New feature (non-breaking change which adds functionality)
• [ ] Breaking change (fix or feature that would cause existing functionality to change)

Checklist

• [x] I have read the contributing document.
• [ ] My change requires a change to the documentation.
• [ ] I have updated the documentation accordingly.
• [ ] I have added tests to cover my changes.
• [ ] All new and existing tests passed.

---

PR Type

Enhancement, Bug fix

---

Description

• Added type hints across multiple files for improved static type checking.

• Enhanced code readability and maintainability by aligning with modern Python practices.

• Fixed potential issues with type safety and compatibility with tools like MyPy and Pyright.

• Refactored code for better clarity, including breaking long lines and improving formatting.

---

Changes walkthrough 📝

Relevant files

Enhancement

options.py Add type hints and improve formatting in options.py            py/selenium/webdriver/common/options.py Added type hints to methods and parameters. Improved formatting by breaking long lines. Enhanced readability with better exception messages. +38/-15  remote_connection.py Add type hints and refactor remote_connection.py                  py/selenium/webdriver/remote/remote_connection.py Added type hints for methods and parameters. Refactored long lines for better readability. Improved exception handling and error messages. +274/-75 script_key.py Add type hints to script_key.py                                                    py/selenium/webdriver/remote/script_key.py Added type hints to class constructor and methods. Enhanced equality checks with proper type annotations. +2/-2      webdriver.py Add type hints and refactor webdriver.py                                  py/selenium/webdriver/remote/webdriver.py Added type hints for methods, parameters, and return types. Refactored long lines and improved formatting. Enhanced error handling and exception messages. +283/-96 webelement.py Add type hints and refactor webelement.py                                py/selenium/webdriver/remote/webelement.py Added type hints for methods and parameters. Improved formatting and readability. Enhanced error handling and exception messages. +69/-29

---

Need help?

Type /help how to ... in the comments thread for any questions about Qodo Merge usage.

Check out the documentation for more information.

[CLAassistant]

All committers have signed the CLA.

[qodo-code-review[bot]]

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 Type Inconsistency The _create_caps function returns a dictionary with capabilities, but in some places like get_remote_connection, the capabilities parameter is used without being created by this function, which could lead to type mismatches. def _create_caps(caps: dict[Any, Any]): """Makes a W3C alwaysMatch capabilities object. Filters out capability names that are not in the W3C spec. Spec-compliant drivers will reject requests containing unknown capability names. Moves the Firefox profile, if present, from the old location to the new Firefox options object. Parameters: ---------- caps : dict - A dictionary of capabilities requested by the caller. """ caps = copy.deepcopy(caps) always_match: dict[Any, Any] = {} firstMatch: list[dict[Any, Any]] = [{}] for k, v in caps.items(): always_match[k] = v capabilities: dict[Any, Any] = { "capabilities": { "firstMatch": firstMatch, "alwaysMatch": always_match, } } return capabilities Conditional Logic The change from else to elif isinstance(options, BaseOptions) in the options handling could potentially miss cases where options is neither a list nor a BaseOptions instance. if isinstance(options, list): capabilities = create_matches(options) _ignore_local_proxy = False elif isinstance(options, BaseOptions): capabilities = options.to_capabilities() _ignore_local_proxy = options._ignore_local_proxy Equality Comparison The __eq__ method now checks if the element is an instance of WebElement, but doesn't return early if the element is None, which could lead to attribute access on None. def __eq__(self, element: object): if not isinstance(element, WebElement): return False return hasattr(element, "id") and self._id == element.id

[qodo-code-review[bot]]

PR Code Suggestions ✨

Explore these optional code suggestions:

Category	Suggestion	Impact
Possible issue	Fix dict modification during iteration The function modifies the input dictionary while iterating over it, which can lead to unexpected behavior. Create a copy of the dictionary before modifying it. py/selenium/webdriver/remote/webdriver.py [406-420] def _unwrap_value(self, value: Any): if isinstance(value, dict): if "element-6066-11e4-a52e-4f735466cecf" in value: return self.create_web_element( value["element-6066-11e4-a52e-4f735466cecf"] ) if "shadow-6066-11e4-a52e-4f735466cecf" in value: return self._shadowroot_cls( self, value["shadow-6066-11e4-a52e-4f735466cecf"] ) + value_dict = value.copy() for key, val in value.items(): - value[key] = self._unwrap_value(val) - - value_dict: dict[Any, Any] = value + value_dict[key] = self._unwrap_value(val) return value_dict [ ] Apply this suggestion Suggestion importance[1-10]: 9 __ Why: This suggestion fixes a critical bug where modifying a dictionary while iterating over it could lead to unpredictable behavior or runtime errors. Creating a copy before modification is a proper solution that prevents potential crashes.	High
	Handle invalid options type The code doesn't handle the case where options is neither a list nor a BaseOptions instance. Add an else clause to handle unexpected types and raise a proper exception. py/selenium/webdriver/remote/webdriver.py [260-262] elif isinstance(options, BaseOptions): capabilities = options.to_capabilities() _ignore_local_proxy = options._ignore_local_proxy +else: + raise TypeError(f"options must be a BaseOptions or list of BaseOptions, not {type(options)}") [ ] Apply this suggestion Suggestion importance[1-10]: 8 __ Why: This suggestion addresses an important error handling gap by adding validation for the options parameter type. Without this check, passing an invalid type would lead to runtime errors without a clear explanation, making debugging difficult.	Medium
General	Fix Self type annotation Using Self as a type annotation for a parameter is incorrect. The Self type should only be used as a return type annotation to indicate that a method returns an instance of the class. For a parameter that can be the same class, you should use the class name directly. py/selenium/webdriver/remote/remote_connection.py [459-467] def __init__( self, - remote_server_addr: Self | str | None = None, + remote_server_addr: 'RemoteConnection' | str | None = None, keep_alive: Optional[bool] = True, ignore_proxy: Optional[bool] = False, ignore_certificates: Optional[bool] = False, proxy_url: Optional[str] = None, client_config: Optional[ClientConfig] = None, ) -> None: [To ensure code accuracy, apply this suggestion manually] Suggestion importance[1-10]: 7 __ Why: This is a valid suggestion that fixes an incorrect type annotation. Using Self as a parameter type is improper - it should only be used as a return type. This correction improves type checking and prevents potential static analysis errors.	Medium
	Fix type checking logic The equality check should verify that the element is a WebElement before accessing its id attribute. While the code checks for the existence of the id attribute, it should first ensure the object is a WebElement to avoid potential attribute errors. py/selenium/webdriver/remote/webelement.py [574-578] def __eq__(self, element: object): if not isinstance(element, WebElement): return False - return hasattr(element, "id") and self._id == element.id + return self._id == element.id [ ] Apply this suggestion Suggestion importance[1-10]: 2 __ Why: The suggestion is technically correct but has minimal impact. The code already checks if the element is a WebElement before proceeding, and the hasattr check is redundant since all WebElement instances will have an id attribute. Removing it simplifies the code slightly but doesn't fix a critical issue.	Low
[ ] Update

[cgoldberg]

Can you remove the formatting changes in this that aren't related to the typing changes? There are spurious newlines and unrelated formating changes.

(running the linting tools will re-format it for you)

[duxtec]

Can you remove the formatting changes in this that aren't related to the typing changes? There are spurious newlines and unrelated formating changes.

(running the linting tools will re-format it for you)

The formatting changes were automatically applied by the Black linter to ensure the code follows the PEP 8 recommendations.

PEP 8 - Maximum Line Length

Which linter should I use to maintain the project's standard formatting, considering it’s a Selenium-related library?

[cgoldberg]

Here's the linters we use:

https://github.com/SeleniumHQ/selenium/blob/030f1f6bd27b99122c5bb57173e47a6390cab99d/py/tox.ini#L64

We use a 120 char line-length with black.

[duxtec]

Can you remove the formatting changes in this that aren't related to the typing changes? There are spurious newlines and unrelated formating changes.

(running the linting tools will re-format it for you)

I've made the requested changes and reverted any unrelated formatting modifications. I also ran the project's linter to ensure consistency.

To facilitate the PR review, I reset the previous commits and created new ones using -f to keep the history clean. Let me know if anything else needs adjustment!

[cgoldberg]

Is this still relevant? If so, can you fix the merge conflicts?

[duxtec]

Yes, this is still relevant as type annotations are still missing. I've resolved the merge conflicts. Let me know if any further adjustments are needed.

[DeflateAwning]

Just a heads up, I think this PR uses conventions not available in old Python (3.8 and/or 3.9), which are supported by Selenium.

An example is that x: str | None must be written as x: Optional[str] or x: Union[str, None] for backwards compatibility with the Python versions supported here.

[cgoldberg]

Thanks for the heads up. We need to be compatible back to 3.9. I just kicked off the CI tests, so we will see if anything fails with 3.9.

[duxtec]

Just a heads up, I think this PR uses conventions not available in old Python (3.8 and/or 3.9), which are supported by Selenium.

An example is that x: str | None must be written as x: Optional[str] or x: Union[str, None] for backwards compatibility with the Python versions supported here.

Thank you for the feedback!

I initially thought that from future import annotations would enable support for the str | None syntax in Python 3.9, but after revisiting the documentation I realized that it's only available from Python 3.10 onward.

I'll update the code to restore compatibility with Python 3.9 by replacing the newer type hint syntax with Optional[...] or Union[...] where needed, and push a new update to this PR shortly.

Thanks again for pointing this out!

[cgoldberg]

There are also quite a few linting failures in this branch. Please run the linters locally and fix any issues when you update your branch. You can run /scripts/format.sh from the main directory in the repo.