Skip to content

[py] fix doc for find_elements #15411

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Merged
merged 6 commits into from
Mar 20, 2025
Merged

Conversation

Delta456
Copy link
Member

@Delta456 Delta456 commented Mar 13, 2025

User description

Thanks for contributing to Selenium!
A PR well described will help maintainers to quickly review and merge it

Before submitting your PR, please check our contributing guidelines.
Avoid large PRs, help reviewers by making them as simple and short as possible.

Motivation and Context

Fixes docstring for find_elements

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

Bug fix, Documentation


Description

  • Corrected example code in find_elements docstrings across multiple files.

  • Ensured consistency in documentation for find_elements method.

  • Improved clarity and accuracy of usage examples for developers.


Changes walkthrough 📝

Relevant files
Documentation
shadowroot.py
Corrected `find_elements` example in docstring                     

py/selenium/webdriver/remote/shadowroot.py

  • Updated example in find_elements docstring to use find_elements
    instead of find_element.
  • Improved clarity in the method's usage documentation.
  • +1/-1     
    webdriver.py
    Updated `find_elements` example in docstring                         

    py/selenium/webdriver/remote/webdriver.py

  • Fixed example in find_elements docstring to correctly use
    find_elements.
  • Enhanced documentation consistency for the method.
  • +1/-1     
    webelement.py
    Fixed `find_elements` example in docstring                             

    py/selenium/webdriver/remote/webelement.py

  • Adjusted example in find_elements docstring to use find_elements
    correctly.
  • Improved clarity and accuracy in the method's documentation.
  • +1/-1     

    Need help?
  • Type /help how to ... in the comments thread for any questions about Qodo Merge usage.
  • Check out the documentation for more information.
  • Delta456 added 3 commits March 6, 2025 16:59
    This reverts commit f17dd08dd32998a310510f0abc44e5b484202a4d.
    Copy link
    Contributor

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

    ⏱️ Estimated effort to review: 1 🔵⚪⚪⚪⚪
    🧪 No relevant tests
    🔒 No security concerns identified
    ⚡ Recommended focus areas for review

    Return Type

    The docstring indicates that the method returns a single WebElement, but find_elements should return a list of WebElements.

    -------
    WebElement
    Return Type

    The docstring indicates that the method returns a single WebElement, but find_elements should return a list of WebElements.

    -------
    WebElement
    Return Type

    The docstring indicates that the method returns a single WebElement, but find_elements should return a list of WebElements.

    -------
    WebElement

    Copy link
    Contributor

    qodo-merge-pro bot commented Mar 13, 2025

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Impact
    General
    Use plural variable name

    The example should assign the result to a variable with a plural name to
    indicate it's a collection of elements, not a single element.

    py/selenium/webdriver/remote/shadowroot.py [100]

    -element = driver.find_elements(By.ID, 'foo')
    +elements = driver.find_elements(By.ID, 'foo')
    • Apply this suggestion
    Suggestion importance[1-10]: 7

    __

    Why: The suggestion correctly points out that since find_elements returns a collection of elements, using a plural variable name (elements) better reflects the nature of the returned value and improves code readability and self-documentation.

    Medium
    • Update

    Copy link
    Contributor

    @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 👍

    Copy link
    Contributor

    @shbenzer shbenzer left a comment

    Choose a reason for hiding this comment

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

    Good catch. Lets also improve the Returns sections:

            Returns:
            -------
            WebElement
                list of `WebElements` matching locator strategy found on the page.
            """
    

    should instead be:

            Returns:
            -------
            List
                list of `WebElements` matching locator strategy found on the page
            """
    

    @Delta456 Delta456 requested a review from shbenzer March 18, 2025 12:48
    @Delta456
    Copy link
    Member Author

    Thanks @shbenzer for the suggestion. I have done the change

    Copy link
    Contributor

    @shbenzer shbenzer left a comment

    Choose a reason for hiding this comment

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

    Thanks @Delta456

    @VietND96 VietND96 merged commit 05ab017 into SeleniumHQ:trunk Mar 20, 2025
    16 of 17 checks passed
    sandeepsuryaprasad pushed a commit to sandeepsuryaprasad/selenium that referenced this pull request Mar 23, 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.

    4 participants