Skip to content

Conversation

@rpallavisharma
Copy link
Member

@rpallavisharma rpallavisharma commented May 29, 2025

User description

🔗 Related Issues

💥 What does this PR do?

fixed selenium/webdriver/common/bidi/common.py:19

🔧 Implementation Notes

💡 Additional Considerations

🔄 Types of changes

  • Cleanup (formatting, renaming)
  • Bug fix (backwards compatible)
  • New feature (non-breaking change which adds functionality and tests!)
  • Breaking change (fix or feature that would cause existing functionality to change)

PR Type

Bug fix


Description

  • Corrects the function signature for command_builder

  • Adds type hints for generator and optional parameters

  • Improves BiDi protocol command construction typing


Changes walkthrough 📝

Relevant files
Bug fix
common.py
Update command_builder signature and type hints                   

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

  • Adds Generator and Optional imports from typing
  • Updates command_builder signature to use generator return type
  • Specifies params as Optional[dict] in function signature
  • +2/-2     

    Need help?
  • Type /help how to ... in the comments thread for any questions about Qodo Merge usage.
  • Check out the documentation for more information.
  • @selenium-ci selenium-ci added C-py Python Bindings B-devtools Includes everything BiDi or Chrome DevTools related labels May 29, 2025
    @qodo-merge-pro
    Copy link
    Contributor

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

    🎫 Ticket compliance analysis ❌

    5678 - Not compliant

    Non-compliant requirements:

    • Fix "Error: ConnectFailure (Connection refused)" when instantiating ChromeDriver

    Requires further human verification:

    • The PR changes BiDi protocol command construction typing, which doesn't appear to be directly related to the ChromeDriver connection issue

    1234 - Not compliant

    Non-compliant requirements:

    • Fix issue where JavaScript in link's href is not triggered on click() in Firefox

    Requires further human verification:

    • The PR changes BiDi protocol command construction typing, which doesn't appear to be directly related to the Firefox click() issue

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

    Return Type Mismatch

    The function signature now indicates it returns a Generator, but the implementation may still return a dict. The implementation should be verified to ensure it actually returns a Generator as specified.

    def command_builder(method: str, params: Optional[dict] = None) -> Generator[dict, dict, dict]:
        """Build a command iterator to send to the BiDi protocol.

    @qodo-merge-pro
    Copy link
    Contributor

    qodo-merge-pro bot commented May 29, 2025

    PR Code Suggestions ✨

    No code suggestions found for the PR.

    @rpallavisharma
    Copy link
    Member Author

    rpallavisharma commented May 29, 2025

    PR- 15697, bug , raised by @cgoldberg . fixing one error in this PR. please review. thanks.

    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. Thanks!

    @cgoldberg cgoldberg merged commit c97f791 into SeleniumHQ:trunk May 29, 2025
    16 checks passed
    @rpallavisharma
    Copy link
    Member Author

    Thank you! :)

    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 1/5

    Projects

    None yet

    Development

    Successfully merging this pull request may close these issues.

    3 participants