[rb] Add RBS type support for BiDi related classes by aguspe · Pull Request #14611 · SeleniumHQ/selenium

aguspe · 2024-10-17T12:57:46Z

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:

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

qodo-code-review · 2024-10-17T13:14:15Z

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.

qodo-code-review · 2024-10-17T13:14:40Z

PR Code Suggestions ✨

Explore these optional code suggestions:

Category	Suggestion	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

aguspe added 2 commits October 17, 2024 14:55

Reduce steep issues from 87 to 65

84e91e8

Update types

a52a22f

aguspe changed the title ~~Reduce steep issues from 87 to 65~~ Reduce steep issues from 87 to 73 Oct 17, 2024

aguspe changed the title ~~Reduce steep issues from 87 to 73~~ [RB] Add RBS type support for BiDi related classes Oct 17, 2024

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 marked this pull request as ready for review October 17, 2024 13:13

qodo-code-review Bot added P-enhancement PR with a new feature documentation Review effort [1-5]: 2 labels Oct 17, 2024

p0deje approved these changes Oct 17, 2024

View reviewed changes

Merge branch 'trunk' into rb_expand_rbs

ab317a3

p0deje merged commit 1b64798 into SeleniumHQ:trunk Oct 18, 2024

titusfortner added B-docs API Docs; see other repo for website documentation and removed documentation labels Mar 30, 2025

M1troll pushed a commit to M1troll/selenium that referenced this pull request May 14, 2025

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

4df48b0

dependabot Bot mentioned this pull request Sep 18, 2025

Bump the selenium group with 2 updates 7474/SRC#677

Merged

dependabot Bot mentioned this pull request Oct 1, 2025

Bump Selenium.Support and Selenium.WebDriver rajbos/github-azure-devops-marketplace-extension-news#277

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

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

[rb] Add RBS type support for BiDi related classes#14611
p0deje merged 3 commits intoSeleniumHQ:trunkfrom
aguspe:rb_expand_rbs

aguspe commented Oct 17, 2024 •

edited by qodo-code-review Bot

Loading

Uh oh!

qodo-code-review Bot commented Oct 17, 2024

Uh oh!

qodo-code-review Bot commented Oct 17, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

Conversation

aguspe commented Oct 17, 2024 • edited by qodo-code-review Bot Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

User description

Description

Motivation and Context

Types of changes

Checklist

PR Type

Description

Changes walkthrough 📝

Uh oh!

qodo-code-review Bot commented Oct 17, 2024

PR Reviewer Guide 🔍

Uh oh!

qodo-code-review Bot commented Oct 17, 2024

PR Code Suggestions ✨

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

aguspe commented Oct 17, 2024 •

edited by qodo-code-review Bot

Loading