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] Expand RBS typing support by replacing untypes with precise typing #13709

Merged
merged 26 commits into from
Jun 4, 2024

Conversation

aguspe
Copy link
Contributor

@aguspe aguspe commented Mar 19, 2024

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

    @aguspe 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
    Copy link

    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.

    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
    Copy link
    Contributor Author

    aguspe commented Jun 2, 2024

    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

    Copy link

    PR Code Suggestions ✨

    CategorySuggestion                                                                                                                                    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
    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
    Copy link
    Contributor Author

    aguspe commented Jun 4, 2024

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

    @titusfortner titusfortner merged commit e672104 into SeleniumHQ:trunk Jun 4, 2024
    30 checks passed
    @titusfortner
    Copy link
    Member

    @aguspe thank you!

    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.

    None yet

    4 participants