Skip to content

Conversation

@shbenzer
Copy link
Contributor

@shbenzer shbenzer commented Jan 9, 2025

User description

Description

Upgraded the docstrings in webdriver.py to be more informative and pep compliant

Motivation and Context

better documentation, educating users, coding standard compliance

Types of changes

  • 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

  • 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

Documentation


Description

  • Upgraded docstrings in webdriver.py for clarity and PEP compliance.

  • Replaced :Args: and :Usage: sections with Parameters and Example.

  • Improved formatting and added detailed examples for methods.

  • Enhanced descriptions for attributes and return types.


Changes walkthrough 📝

Relevant files
Documentation
webdriver.py
Enhanced docstrings for WebDriver methods and attributes 

py/selenium/webdriver/remote/webdriver.py

  • Replaced :Args: and :Usage: with Parameters and Example.
  • Added detailed examples for methods and attributes.
  • Improved descriptions for attributes and return types.
  • Enhanced formatting for better readability and PEP compliance.
  • +314/-249

    💡 PR-Agent usage: Comment /help "your question" on any pull request to receive relevant information

    @qodo-merge-pro
    Copy link
    Contributor

    qodo-merge-pro bot commented Jan 9, 2025

    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

    Documentation Gap

    Some methods still lack complete parameter descriptions and return type documentation, like pin_script() and get_pinned_scripts(). Consider adding full documentation for all public methods.

    def pin_script(self, script: str, script_key=None) -> ScriptKey:
        """Store common javascript scripts to be executed later by a unique
        hashable ID."""
        script_key_instance = ScriptKey(script_key)
        self.pinned_scripts[script_key_instance.id] = script
        return script_key_instance
    
    def unpin(self, script_key: ScriptKey) -> None:
        """Remove a pinned script from storage."""
        try:
            self.pinned_scripts.pop(script_key.id)
        except KeyError:
            raise KeyError(f"No script with key: {script_key} existed in {self.pinned_scripts}") from None
    
    def get_pinned_scripts(self) -> List[str]:
        return list(self.pinned_scripts)
    Formatting Issue

    Some docstrings have inconsistent indentation and newline usage in the Parameters and Example sections. Consider standardizing the formatting across all docstrings.

    Parameters:
    ----------
    command_executor : str or remote_connection.RemoteConnection
        - Either a string representing the URL of the remote server or a custom
        remote_connection.RemoteConnection object. Defaults to 'http://127.0.0.1:4444/wd/hub'.
    keep_alive : bool (Deprecated)
        - Whether to configure remote_connection.RemoteConnection to use HTTP keep-alive. Defaults to True.
    file_detector : object or None
        - Pass a custom file detector object during instantiation. If None, the default LocalFileDetector() will be used.
    options : options.Options
        - Instance of a driver options.Options class.
    locator_converter : object or None
        - Custom locator converter to use. Defaults to None.
    web_element_cls : class
        - Custom class to use for web elements. Defaults to WebElement.
    client_config : object or None
        - Custom client configuration to use. Defaults to None.

    @qodo-merge-pro
    Copy link
    Contributor

    qodo-merge-pro bot commented Jan 9, 2025

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Score
    Possible issue
    Add error handling for network issues and timeouts in page navigation

    The get() method should handle potential network errors and timeouts. Add error
    handling to provide better feedback when page loading fails.

    py/selenium/webdriver/remote/webdriver.py [424-441]

     def get(self, url: str) -> None:
         """Navigate the browser to the specified URL in the current window or tab.
     
         The method does not return until the page is fully loaded (i.e. the
         onload event has fired).
     
         Parameters:
         ----------
         url : str
             - The URL to be opened by the browser.
             - Must include the protocol (e.g., http://, https://).
     
         Example:
         --------
         >>> driver = webdriver.Chrome()
         >>> driver.get("https://example.com")
    +
    +    Raises:
    +    -------
    +    TimeoutException: If page load timeout is exceeded
    +    WebDriverException: If navigation fails
         """
    -    self.execute(Command.GET, {"url": url})
    +    try:
    +        self.execute(Command.GET, {"url": url})
    +    except TimeoutException:
    +        raise TimeoutException(f"Timeout exceeded while loading {url}")
    +    except WebDriverException as e:
    +        raise WebDriverException(f"Failed to load {url}: {str(e)}")
    • Apply this suggestion
    Suggestion importance[1-10]: 8

    Why: The suggestion adds important error handling for common page load failures, improving error reporting and debuggability. The enhanced error messages would help users better understand navigation failures.

    8
    Add validation for window dimension parameters to prevent invalid values

    The set_window_rect() method should validate input parameters to prevent invalid
    window dimensions that could cause the browser to misbehave.

    py/selenium/webdriver/remote/webdriver.py [1066-1067]

     if (x is None and y is None) and (not height and not width):
         raise InvalidArgumentException("x and y or height and width need values")
    +if width is not None and width <= 0:
    +    raise InvalidArgumentException("Window width must be positive")
    +if height is not None and height <= 0:
    +    raise InvalidArgumentException("Window height must be positive")
    • Apply this suggestion
    Suggestion importance[1-10]: 7

    Why: The suggestion adds important input validation to prevent setting invalid window dimensions that could cause browser issues. This helps prevent runtime errors and improves API robustness.

    7
    General
    Fix documentation typo in parameter description

    The fedcm_dialog() method has incorrect parameter documentation - there's a typo in
    the poll_frequency description that splits "How Frequently" into two lines.

    py/selenium/webdriver/remote/webdriver.py [1395-1396]

    -poll_frequency : floatHow 
    -    - Frequently to poll
    +poll_frequency : float
    +    - How frequently to poll for dialog presence
    • Apply this suggestion
    Suggestion importance[1-10]: 5

    Why: The suggestion fixes a documentation formatting error that affects readability. While not a functional issue, clear documentation is important for API usability.

    5

    @shbenzer shbenzer changed the title Upgraded WebDriver Docstrings [py] Upgraded WebDriver Docstrings Jan 9, 2025
    @VietND96 VietND96 merged commit a7c0cfb into SeleniumHQ:trunk Jan 14, 2025
    17 checks passed
    @shbenzer shbenzer deleted the webdriver-docstrings branch January 16, 2025 00:38
    sandeepsuryaprasad pushed a commit to sandeepsuryaprasad/selenium that referenced this pull request Mar 23, 2025
    gryznar pushed a commit to gryznar/selenium that referenced this pull request May 17, 2025
    Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

    Projects

    None yet

    Development

    Successfully merging this pull request may close these issues.

    3 participants