Skip to content
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

[rb] Add RBS type support for BiDi related classes #14611

Merged
merged 3 commits into from
Oct 18, 2024

Conversation

aguspe
Copy link
Contributor

@aguspe aguspe commented Oct 17, 2024

User description

Description

Continuing the goal of adding full type support stated on the feature #10943 this PR adds rbs files for newly created BiDi classes and update types

This PR reduces the errors from 87 to 73:

Screenshot 2024-10-17 at 15 09 03 Screenshot 2024-10-17 at 14 54 19

Motivation and Context

The goal is that the Ruby Binding has good type support coverage and that we can perform type validations on our CI/CD

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

enhancement, documentation


Description

  • Added new RBS files and updated existing ones to enhance type support for BiDi-related classes.
  • Introduced LogHandler class to manage logging within the BiDi module.
  • Added Struct class with a utility method for string conversion.
  • Implemented BiDiBridge class for managing BiDi sessions.
  • Enhanced Bridge class with a new bidi method.
  • Introduced LocatorConverter class for handling locator conversions.

Changes walkthrough 📝

Relevant files
Enhancement
bidi.rbs
Add callback management and update command method signature

rb/sig/lib/selenium/webdriver/bidi.rbs

  • Added add_callback and remove_callback methods.
  • Updated send_cmd method to accept a String type for the method
    parameter.
  • +5/-1     
    log_handler.rbs
    Introduce LogHandler class for BiDi logging                           

    rb/sig/lib/selenium/webdriver/bidi/log_handler.rbs

  • Introduced LogHandler class within BiDi.
  • Added methods for message handler management.
  • Included private methods for log entry subscription.
  • +27/-0   
    struct.rbs
    Add Struct class with camel to snake conversion                   

    rb/sig/lib/selenium/webdriver/bidi/struct.rbs

  • Added Struct class with a method to convert camel case to snake case.
  • +11/-0   
    bidi_bridge.rbs
    Add BiDiBridge class for session management                           

    rb/sig/lib/selenium/webdriver/remote/bidi_bridge.rbs

  • Introduced BiDiBridge class extending Bridge.
  • Added methods for session creation and termination.
  • +17/-0   
    bridge.rbs
    Add bidi method to Bridge class                                                   

    rb/sig/lib/selenium/webdriver/remote/bridge.rbs

    • Added bidi method returning WebDriverError.
    +2/-0     
    locator_converter.rbs
    Add LocatorConverter class for locator conversion               

    rb/sig/lib/selenium/webdriver/remote/bridge/locator_converter.rbs

  • Introduced LocatorConverter class within Bridge.
  • Added method for converting locators.
  • +19/-0   

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

    @aguspe aguspe changed the title Reduce steep issues from 87 to 65 Reduce steep issues from 87 to 73 Oct 17, 2024
    @aguspe aguspe changed the title Reduce steep issues from 87 to 73 [RB] Add RBS type support for BiDi related classes Oct 17, 2024
    @aguspe aguspe changed the title [RB] Add RBS type support for BiDi related classes [rb] Add RBS type support for BiDi related classes Oct 17, 2024
    @aguspe aguspe marked this pull request as ready for review October 17, 2024 13:13
    Copy link
    Contributor

    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

    Type Inconsistency
    The send_cmd method's method parameter type has been changed from untyped to String. Verify if this change is intentional and consistent with the method's implementation.

    Incomplete Type Definitions
    The ConsoleLogEntry and JavaScriptLogEntry are defined as Struct without specifying their attributes. Consider adding more detailed type information for these structures.

    Untyped Attribute
    The bidi attribute is defined as untyped. Consider specifying a more precise type for better type checking and documentation.

    Copy link
    Contributor

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Score
    Enhancement
    Align the type of the bidi instance variable with its usage in the create_session method

    The create_session method returns a BiDi object, but the @bidi instance variable is
    typed as untyped. Consider updating the type of @bidi to BiDi for consistency and
    improved type safety.

    rb/sig/lib/selenium/webdriver/remote/bidi_bridge.rbs [5-9]

    -@bidi: untyped
    +@bidi: BiDi
     
    -attr_reader bidi: untyped
    +attr_reader bidi: BiDi
     
     def create_session: (Hash[Symbol, String] capabilities) -> BiDi
    • Apply this suggestion
    Suggestion importance[1-10]: 6

    Why: Updating the type of the @bidi instance variable to BiDi enhances type safety and consistency with the create_session method, making this a valuable improvement for maintaining type integrity.

    6
    Modify the return type of remove_message_handler to represent both success and failure outcomes

    The remove_message_handler method is defined to return false, which might not
    accurately represent all possible outcomes. Consider changing it to return a boolean
    to indicate success or failure of the operation.

    rb/sig/lib/selenium/webdriver/bidi/log_handler.rbs [17]

    -def remove_message_handler: (Integer id) -> false
    +def remove_message_handler: (Integer id) -> bool
    • Apply this suggestion
    Suggestion importance[1-10]: 5

    Why: The suggestion to change the return type of remove_message_handler to a boolean is reasonable, as it would allow the method to indicate both success and failure, improving the method's expressiveness and usability.

    5
    Adjust the return type and parameters of the remove_callback method for consistency

    The remove_callback method is defined to return an Array[Integer], which seems
    inconsistent with add_callback. Consider changing it to return a boolean indicating
    success or failure, or a single Integer representing the removed callback ID.

    rb/sig/lib/selenium/webdriver/bidi.rbs [16]

    -def remove_callback: -> Array[Integer]
    +def remove_callback: (CallbackId id) -> bool
    • Apply this suggestion
    Suggestion importance[1-10]: 4

    Why: Changing the return type of remove_callback to a boolean or a single Integer could enhance consistency and clarity, but the suggestion lacks specific context on how the method is used, limiting its immediate applicability.

    4
    Specify a more precise return type for the add_callback method

    Consider specifying the return type for the add_callback method. It's currently
    defined to return an Integer, but it might be more specific, such as a callback ID
    or a status code.

    rb/sig/lib/selenium/webdriver/bidi.rbs [10]

    -def add_callback: -> Integer
    +def add_callback: -> CallbackId
    • Apply this suggestion
    Suggestion importance[1-10]: 3

    Why: The suggestion to specify a more precise return type for the add_callback method could improve code clarity and type safety, but without additional context on what CallbackId represents, the impact is limited.

    3

    💡 Need additional feedback ? start a PR chat

    @p0deje p0deje merged commit 1b64798 into SeleniumHQ:trunk Oct 18, 2024
    1 check passed
    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.

    2 participants