Skip to content

Conversation

iampopovich
Copy link
Contributor

@iampopovich iampopovich commented Oct 19, 2025

User description

🔗 Related Issues

relates to #11442

💥 What does this PR do?

This pull request updates docstrings throughout the codebase to use the Google-style Args and Returns format, replacing the previous :param and :returns syntax. The changes improve consistency and readability of documentation for classes and methods in several modules, including cdp.py, script.py, driver_finder.py, options.py, and service.py.

Docstring style modernization:

  • Updated all constructor and method docstrings in py/selenium/webdriver/common/bidi/cdp.py to use Args and Returns sections instead of :param and :returns, enhancing clarity for parameters and return values. [1] [2] [3] [4] [5] [6]
  • Refactored docstrings for data classes and methods in py/selenium/webdriver/common/bidi/script.py to use the new format, including constructors and JSON deserialization methods. [1] [2] [3] [4] [5] [6] [7] [8] [9] [10] [11] [12] [13] [14] [15]
  • Updated class-level docstrings in py/selenium/webdriver/common/driver_finder.py to use Args.
  • Modernized docstrings for descriptor classes and methods in py/selenium/webdriver/common/options.py, including consistent use of Args and Returns and improved argument documentation. [1] [2] [3] [4] [5] [6]
  • Improved docstring parameter documentation for the Service class in py/selenium/webdriver/common/service.py.

🔧 Implementation Notes

💡 Additional Considerations

🔄 Types of changes

  • Cleanup (formatting, renaming)

PR Type

Documentation


Description

  • Converted docstrings from :param: and :returns: to Google-style Args and Returns format

  • Updated multiple modules including CDP, script, driver finder, options, and service classes

  • Improved consistency and readability of parameter and return value documentation

  • Added type hints to method signatures in remote connection and script modules


Diagram Walkthrough

flowchart LR
  A["Old Format<br/>:param: and :returns:"] -->|"Convert to"| B["Google Style<br/>Args and Returns"]
  B -->|"Applied to"| C["Multiple Modules<br/>CDP, Script, Service, etc."]
  C -->|"Result"| D["Improved Documentation<br/>Consistency"]
Loading

File Walkthrough

Relevant files
Documentation
9 files
cdp.py
Convert CDP docstrings to Google-style format                       
+21/-13 
script.py
Refactor script module docstrings to Args/Returns               
+15/-44 
driver_finder.py
Update DriverFinder class docstring format                             
+3/-2     
options.py
Modernize options descriptor docstrings                                   
+24/-11 
service.py
Convert Service class docstrings to Google style                 
+11/-9   
firefox_profile.py
Update FirefoxProfile docstrings format                                   
+13/-10 
service.py
Refactor Firefox Service docstring format                               
+7/-6     
remote_connection.py
Modernize RemoteConnection docstrings and add type hints 
+34/-29 
service.py
Update Safari Service docstring format                                     
+8/-7     

@selenium-ci selenium-ci added C-py Python Bindings B-devtools Includes everything BiDi or Chrome DevTools related labels Oct 19, 2025
@iampopovich iampopovich marked this pull request as ready for review October 19, 2025 12:10
Copy link
Contributor

qodo-merge-pro bot commented Oct 19, 2025

PR Compliance Guide 🔍

Below is a summary of compliance checks for this PR:

Security Compliance
Unvalidated input dict access

Description: The method signature for _handle_cmd_response(self, data: dict) now annotates data as dict
but the code accesses data["id"] without validation, which can raise KeyError if untrusted
input is malformed; consider validating keys before indexing.
cdp.py [275-285]

Referred Code
def _handle_cmd_response(self, data: dict):
    """Handle a response to a command. This will set an event flag that
    will return control to the task that called the command.

    Args:
        data: response as a JSON dictionary
    """
    cmd_id = data["id"]
    try:
        cmd, event = self.inflight_cmd.pop(cmd_id)
    except KeyError:
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

@iampopovich
Copy link
Contributor Author

@cgoldberg more files to review . thanks 🙏

Copy link
Contributor

qodo-merge-pro bot commented Oct 19, 2025

PR Code Suggestions ✨

Explore these optional code suggestions:

CategorySuggestion                                                                                                                                    Impact
Learned
best practice
Add validation and type hints

Add input validation and concrete type hints for method, url, and body to guard
against invalid values early. Check that method is one of allowed verbs and that
url is non-empty and well-formed.

py/selenium/webdriver/remote/remote_connection.py [409-422]

-def _request(self, method, url, body=None) -> dict:
+def _request(self, method: str, url: str, body: Optional[str] = None) -> dict:
     """Send an HTTP request to the remote server.
 
     Args:
-        method: A string for the HTTP method to send the request with.
-        url: A string for the URL to send the request to.
-        body: A string for request body. Ignored unless method is POST or PUT.
+        method: HTTP method to send the request with (e.g., 'GET', 'POST', 'PUT', 'DELETE').
+        url: Absolute URL to send the request to.
+        body: Request body as a JSON string. Ignored unless method is POST or PUT.
 
     Returns:
         A dictionary with the server's parsed JSON response.
     """
+    if not isinstance(method, str) or method.upper() not in {"GET", "POST", "PUT", "DELETE"}:
+        raise ValueError(f"Unsupported HTTP method: {method!r}")
+    if not isinstance(url, str) or not url:
+        raise ValueError("URL must be a non-empty string")
+
+    method = method.upper()
     parsed_url = parse.urlparse(url)
     headers = self.get_remote_connection_headers(parsed_url, self._client_config.keep_alive)
     auth_header = self._client_config.get_auth_header()
  • Apply / Chat
Suggestion importance[1-10]: 6

__

Why:
Relevant best practice - Validate inputs and add precise type hints to prevent logic errors and improve API clarity.

Low
Possible issue
Handle list values during log truncation

Modify the _trim_large_entries method to also handle truncation of values within
lists, not just dictionaries, to prevent oversized log entries.

py/selenium/webdriver/remote/remote_connection.py [476-495]

 def _trim_large_entries(self, input_dict, max_length=100) -> dict:
     """Truncate string values in a dictionary if they exceed max_length.
 
     Args:
         input_dict: Dictionary with potentially large values
         max_length: Maximum allowed length of string values
 
     Returns:
         Dictionary with truncated string values
     """
     output_dictionary = {}
     for key, value in input_dict.items():
         if isinstance(value, dict):
             output_dictionary[key] = self._trim_large_entries(value, max_length)
+        elif isinstance(value, list):
+            output_dictionary[key] = [
+                self._trim_large_entries(item, max_length) if isinstance(item, dict) else item for item in value
+            ]
         elif isinstance(value, str) and len(value) > max_length:
             output_dictionary[key] = value[:max_length] + "..."
         else:
             output_dictionary[key] = value
 
     return output_dictionary
  • Apply / Chat
Suggestion importance[1-10]: 5

__

Why: The suggestion correctly identifies that the _trim_large_entries method does not handle lists, which could lead to very long log lines. However, the proposed improved_code is incomplete as it only handles dictionaries within lists and fails to truncate long strings that are also inside lists.

Low
  • Update

Copy link
Member

@cgoldberg cgoldberg left a comment

Choose a reason for hiding this comment

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

LGTM 👍

@cgoldberg cgoldberg merged commit 81b9850 into SeleniumHQ:trunk Oct 19, 2025
1 check 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.

3 participants