[rb] Expand RBS typing support by replacing untypes with precise typing by aguspe · Pull Request #13709 · SeleniumHQ/selenium

aguspe · 2024-03-19T19:42:47Z

User description

Description

This PR is the first PR of several to replace the untyped types for the precise types in all of the RBS signature files

The goal of splitting this process into multiple PRs is to make it easier for reviewers to go through the PRs, if anyone will prefer a big PR instead please let me know

Motivation and Context

Based on #10943 I created the following PRs to add RBS-type support to the ruby selenium library:

On #12844 I started adding support for RBS and I added the steep file configuration

On #13192 I extended the classes supported on RBS and updated the logger file

On #13234 I added RBS files for the entire selenium library,
however several of the autogenerated files only have the type defined as untyped instead of the precise type

Example

Before:

  def atom_script: (untyped) -> untyped

  private

  def read_atom: (Symbol function) -> String

  def execute_atom: (Symbol function_name, *untyped arguments) -> untyped

After:

  def atom_script: (Symbol) -> String

  private

  def read_atom: (Symbol function) -> String

  def execute_atom: (Symbol function_name, [Element | String | Symbol] arguments) -> [Element | Integer | Float | bool | nil | String | Array[untyped]]

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

Description

Replaced untyped attributes and method parameters with specific types across multiple files.
Updated method signatures to use specific types instead of untyped.
Enhanced type safety and clarity in the RBS signature files for the Selenium library.

Changes walkthrough 📝

Relevant files

Enhancement

10 files

server.rbs `Replace untyped attributes and methods with specific types` rb/sig/lib/selenium/server.rbs Replaced untyped attributes with specific types (e.g., `String`, `Integer`, `bool`). Updated method signatures to use specific types instead of `untyped`.	+12/-12
atoms.rbs `Update method signatures with specific types` rb/sig/lib/selenium/webdriver/atoms.rbs Updated method signatures to use specific types instead of `untyped`.	+2/-2
bidi.rbs `Replace untyped attributes and methods with specific types` rb/sig/lib/selenium/webdriver/bidi.rbs Replaced untyped attributes with specific types (e.g., `WebSocketConnection`, `Session`). Updated method signatures to use specific types instead of `untyped`.	+7/-7
features.rbs `Update constants and methods with specific types` rb/sig/lib/selenium/webdriver/chrome/features.rbs Updated constants and method signatures to use specific types instead of `untyped`.	+3/-3
options.rbs `Replace untyped attributes and methods with specific types` rb/sig/lib/selenium/webdriver/common/options.rbs Replaced untyped attributes and method parameters with specific types (e.g., `Hash`, `String`, `Symbol`). Updated method signatures to use specific types instead of `untyped`.	+18/-17
target_locator.rbs `Update method signatures with specific types` rb/sig/lib/selenium/webdriver/common/target_locator.rbs Updated method signatures to use specific types instead of `untyped`.	+2/-2
devtools.rbs `Replace untyped attributes and methods with specific types` rb/sig/lib/selenium/webdriver/devtools.rbs Replaced untyped attributes and method parameters with specific types (e.g., `Hash`). Updated method signatures to use specific types instead of `untyped`.	+2/-2
edge.rbs `Replace untyped attributes and methods with specific types` rb/sig/lib/selenium/webdriver/edge.rbs Replaced untyped attributes and method parameters with specific types (e.g., `String`).	+3/-3
firefox.rbs `Replace untyped attributes and methods with specific types` rb/sig/lib/selenium/webdriver/firefox.rbs Replaced untyped attributes and method parameters with specific types (e.g., `String`).	+3/-3
safari.rbs `Replace untyped attributes and methods with specific types` rb/sig/lib/selenium/webdriver/safari.rbs Replaced untyped attributes and method parameters with specific types (e.g., `String`, `bool`).	+7/-8

💡 PR-Agent usage:
Comment /help on the PR to get a list of all available PR-Agent tools and their descriptions

… into expand_rbs_type_checking

qodo-code-review · 2024-06-02T08:39:23Z

PR Review 🔍

⏱️ Estimated effort to review [1-5]	2, because the changes are mostly type annotations and straightforward replacements of untyped with specific types. The PR is large but the nature of changes is repetitive and mostly involves updating method signatures with more precise types, which is generally less complex than logic changes.
🧪 Relevant tests	No
⚡ Possible issues	Possible Type Mismatch: The PR introduces specific types where previously 'untyped' was used. This could potentially lead to type mismatches if not all usages of these methods have been updated accordingly or if the inferred types are incorrect.
⚡ Possible issues	Incomplete Typing: Some elements still use 'untyped' within arrays or hashes, e.g., `Array[untyped]` or `Hash[untyped, untyped]`. This might indicate incomplete typing or areas where typing could be further improved.
🔒 Security concerns	No

aguspe · 2024-06-02T08:39:48Z

Once this PR is merged I will keep improving the RBS-type support and in the meantime, I will work on my draft #13796 and I will add the correspondent RBS files for all the new classes and modules

qodo-code-review · 2024-06-02T08:39:50Z

PR Code Suggestions ✨

Category	Suggestion	Score
Type safety	Specify the types for the `method` and `params` parameters in the `send_cmd` method for better type safety The `send_cmd` method still uses `untyped` for both `method` and `params`. For better type safety and clarity, consider specifying the expected types for these parameters. rb/sig/lib/selenium/webdriver/bidi.rbs [16] -def send_cmd: (untyped method, untyped params) -> untyped +def send_cmd: (String method, Hash[String, untyped] params) -> untyped Suggestion importance[1-10]: 8 Why: This suggestion addresses a significant improvement in type safety by specifying types for parameters in a method that currently uses `untyped`, which can greatly enhance code clarity and error prevention.	8
Type safety	Specify the types within the array for the `arguments` parameter in the `execute_atom` method The `execute_atom` method currently uses an array of `untyped` for the `arguments` parameter. For better type safety, consider specifying the types within the array. rb/sig/lib/selenium/webdriver/atoms.rbs [13] -def execute_atom: (Symbol function_name, [Element \| String \| Symbol] arguments) -> [Element \| Integer \| Float \| bool \| nil \| String \| Array[untyped]] +def execute_atom: (Symbol function_name, [Element \| String \| Symbol] arguments) -> [Element \| Integer \| Float \| bool \| nil \| String \| Array[Element \| String \| Symbol]] Suggestion importance[1-10]: 5 Why: The suggestion to specify types within the array for better type safety is valid, but the existing code already includes specific types in the array, making this suggestion somewhat redundant.	5
Possible issue	Change the type of `@additional_args` to allow a combination of integers, booleans, and untyped elements within the array The `@additional_args` attribute is currently typed as `Integer \| bool \| Array[untyped]`. It would be more flexible and accurate to allow for a combination of these types within the array. Consider changing it to `Array[Integer \| bool \| untyped]`. rb/sig/lib/selenium/server.rbs [19] -@additional_args: Integer \| bool \| Array[untyped] +@additional_args: Array[Integer \| bool \| untyped] Suggestion importance[1-10]: 7 Why: The suggestion correctly identifies a potential improvement in type flexibility for the `@additional_args` attribute, which can enhance the code's robustness and clarity.	7
Best practice	Update the return type of `add_option` to include `nil` for better accuracy The `add_option` method currently allows `nil` values for the `value` parameter, but the return type does not reflect this. Consider updating the return type to include `nil`. rb/sig/lib/selenium/webdriver/common/options.rbs [38] -def add_option: (String \| Symbol name, String \| Numeric \| bool? value) -> (String \| Numeric \| bool)? +def add_option: (String \| Symbol name, String \| Numeric \| bool? value) -> (String \| Numeric \| bool \| nil)? Suggestion importance[1-10]: 6 Why: This suggestion is accurate in ensuring that the method signature reflects all possible return types, including `nil`, which improves the method's type accuracy.	6

aguspe · 2024-06-04T04:15:44Z

Hi @p0deje I hope you are having a great day, if you have time could you review this PR? thank you so much

titusfortner · 2024-06-04T16:57:38Z

@aguspe thank you!

…ng (SeleniumHQ#13709) * Update atoms.rb signature types * Start adding bidi types * Extended types of devtools and edge * Added firefox and safari types and removed unused signature * Update chrome features with type and remove Safari error * Update server and remove unused signature * Expand Server type signature * expanding type support on options * Expand options types * add extra options support * Expand options support --------- Co-authored-by: aguspe <agustin.pe94@gmail.com>

aguspe and others added 16 commits March 19, 2024 20:16

Update atoms.rb signature types

ec59a1e

Start adding bidi types

38cea9c

Extended types of devtools and edge

0c3188d

Merge branch 'trunk' into expand_rbs_type_checking

076df94

Added firefox and safari types and removed unused signature

74c6000

Merge branch 'expand_rbs_type_checking' of github.com:aguspe/selenium…

be3887e

… into expand_rbs_type_checking

Merge branch 'trunk' into expand_rbs_type_checking

dbee5f9

Merge branch 'trunk' into expand_rbs_type_checking

e5e2d41

Update chrome features with type and remove Safari error

6cb1eca

Update server and remove unused signature

77563a7

Merge branch 'trunk' into expand_rbs_type_checking

447a2c6

Expand Server type signature

b0295de

Merge branch 'expand_rbs_type_checking' of github.com:aguspe/selenium…

841e148

… into expand_rbs_type_checking

expanding type support on options

e819592

Merge branch 'trunk' into expand_rbs_type_checking

7ae628c

Expand options types

66a8e62

aguspe changed the title ~~Expand RBS typing support by replacing untypes with precise typing~~ [rb] Expand RBS typing support by replacing untypes with precise typing Apr 13, 2024

aguspe and others added 8 commits April 13, 2024 22:34

Merge branch 'trunk' into expand_rbs_type_checking

ab76a40

Merge branch 'trunk' into expand_rbs_type_checking

3d5361b

add extra options support

68757f0

Merge branch 'trunk' into expand_rbs_type_checking

ddd43cd

Merge branch 'trunk' into expand_rbs_type_checking

fc858f2

Expand options support

7bcf60b

Merge branch 'expand_rbs_type_checking' of github.com:aguspe/selenium…

01ddcf7

… into expand_rbs_type_checking

Merge branch 'trunk' into expand_rbs_type_checking

ecf5687

aguspe marked this pull request as ready for review June 2, 2024 08:38

qodo-code-review bot added P-enhancement PR with a new feature Review effort [1-5]: 2 labels Jun 2, 2024

Merge branch 'trunk' into expand_rbs_type_checking

23908a5

Merge branch 'trunk' into expand_rbs_type_checking

f51e5b7

p0deje approved these changes Jun 4, 2024

View reviewed changes

titusfortner merged commit e672104 into SeleniumHQ:trunk Jun 4, 2024

seedysoft mentioned this pull request Jun 21, 2024

Bump Selenium.WebDriver from 4.21.0 to 4.22.0 seedysoft/all-my-stuff#53

Merged

MiguelPelegrina mentioned this pull request Jul 12, 2024

[Snyk] Upgrade selenium-webdriver from 4.21.0 to 4.22.0 MiguelPelegrina/java_school_online_store_frontend#19

Merged

aravindb26 mentioned this pull request Jul 16, 2024

[Snyk] Upgrade selenium-webdriver from 4.16.0 to 4.22.0 aravindb26/spark#3

Open

x-way mentioned this pull request Jul 24, 2024

[Snyk] Upgrade selenium-webdriver from 4.18.1 to 4.22.0 x-way/cal-heatmap#503

Closed

x-way mentioned this pull request Aug 10, 2024

[Snyk] Upgrade selenium-webdriver from 4.18.1 to 4.23.0 x-way/cal-heatmap#544

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

[rb] Expand RBS typing support by replacing untypes with precise typing#13709

[rb] Expand RBS typing support by replacing untypes with precise typing#13709
titusfortner merged 26 commits intoSeleniumHQ:trunkfrom
aguspe:expand_rbs_type_checking

aguspe commented Mar 19, 2024 •

edited

Loading

Uh oh!

qodo-code-review bot commented Jun 2, 2024

Uh oh!

aguspe commented Jun 2, 2024

Uh oh!

qodo-code-review bot commented Jun 2, 2024

Uh oh!

aguspe commented Jun 4, 2024

Uh oh!

titusfortner commented Jun 4, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Uh oh!

Conversation

aguspe commented Mar 19, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

User description

Description

Motivation and Context

Example

Types of changes

Checklist

PR Type

Description

Changes walkthrough 📝

Uh oh!

qodo-code-review bot commented Jun 2, 2024

PR Review 🔍

Uh oh!

aguspe commented Jun 2, 2024

Uh oh!

qodo-code-review bot commented Jun 2, 2024

PR Code Suggestions ✨

Uh oh!

aguspe commented Jun 4, 2024

Uh oh!

titusfortner commented Jun 4, 2024

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

aguspe commented Mar 19, 2024 •

edited

Loading