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] Reduce RBS errors to 0 #14661

Open
wants to merge 3 commits into
base: trunk
Choose a base branch
from

Conversation

aguspe
Copy link
Contributor

@aguspe aguspe commented Oct 27, 2024

User description

Description

This PRs implements inline ignore comments for issues related to the steep checker and updates the types that were causing errors

Screenshot 2024-11-01 at 23 37 45 Screenshot 2024-11-01 at 23 36 52

Motivation and Context

To add full RBS support for Selenium and eventually allow us to add an RBS check on the pipeline.
The reference feature is #10943

The goal is also to be able to have no steep errors to start adding the right type on the classes that have untyped to have the right type enforces

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

  • Added steep ignore comments to various Ruby files to handle type checking issues.
  • Updated RBS files to improve type definitions and reduce errors.
  • Enhanced method signatures and attributes to use more appropriate types.
  • Improved documentation and comments for better code clarity.

Changes walkthrough 📝

Relevant files
Enhancement
20 files
log_handler.rb
Add steep ignore comments for log handler method                 

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

  • Added steep ignore comments around add_message_handler method.
+2/-0     
account.rb
Add steep ignore comments for account initialization         

rb/lib/selenium/webdriver/common/fedcm/account.rb

  • Added steep ignore comments around the initialize method.
+2/-3     
target_locator.rb
Add steep ignore comments for new window method                   

rb/lib/selenium/webdriver/common/target_locator.rb

  • Added steep ignore comments around new_window method.
+2/-0     
common.rb
Add steep ignore comments for HTTP call method                     

rb/lib/selenium/webdriver/remote/http/common.rb

  • Added steep ignore comments around call method.
+2/-2     
bidi.rbs
Update add_callback method signature in BiDi                         

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

  • Updated add_callback method signature to include event parameter.
  • +1/-1     
    log_handler.rbs
    Update log handler method signatures                                         

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

  • Made add_message_handler block parameter optional.
  • Updated return type of remove_message_handler.
  • +2/-2     
    struct.rbs
    Specify untyped arguments for Struct class                             

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

    • Specified Struct class to use untyped arguments.
    +1/-1     
    driver_finder.rbs
    Add to_args method to driver finder                                           

    rb/sig/lib/selenium/webdriver/common/driver_finder.rbs

    • Added to_args method definition.
    +2/-0     
    error.rbs
    Allow optional keys in URLS hash                                                 

    rb/sig/lib/selenium/webdriver/common/error.rbs

    • Updated URLS hash to allow optional keys.
    +1/-1     
    logger.rbs
    Make block parameters optional in logger methods                 

    rb/sig/lib/selenium/webdriver/common/logger.rbs

  • Made block parameters optional in deprecate and discard_or_log
    methods.
  • +2/-2     
    options.rbs
    Use untyped keys and values for options hash                         

    rb/sig/lib/selenium/webdriver/common/options.rbs

    • Changed @options hash to use untyped keys and values.
    +1/-1     
    selenium_manager.rbs
    Update binary_paths method to accept variable arguments   

    rb/sig/lib/selenium/webdriver/common/selenium_manager.rbs

    • Updated binary_paths method to accept variable string arguments.
    +1/-1     
    service.rbs
    Add DRIVER_PATH_ENV_KEY constant                                                 

    rb/sig/lib/selenium/webdriver/common/service.rbs

    • Added DRIVER_PATH_ENV_KEY constant.
    +2/-0     
    websocket_connection.rbs
    Update add_callback method with block parameter                   

    rb/sig/lib/selenium/webdriver/common/websocket_connection.rbs

    • Updated add_callback method to include block parameter.
    +1/-1     
    account.rbs
    Change initialize method to accept string arguments           

    rb/sig/lib/selenium/webdriver/fedcm/account.rbs

    • Changed initialize method to accept string arguments.
    +1/-21   
    options.rbs
    Update options hash and enable_android parameters               

    rb/sig/lib/selenium/webdriver/firefox/options.rbs

  • Updated @options hash to use untyped keys and values.
  • Specified enable_android method parameters.
  • +2/-2     
    bidi_bridge.rbs
    Update close method return type                                                   

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

    • Updated close method return type to untyped.
    +1/-1     
    bridge.rbs
    Update method signatures in remote bridge                               

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

  • Updated fedcm_account_list method return type.
  • Changed select_fedcm_account method to accept an index parameter.
  • Updated execute method return type to string.
  • +3/-3     
    response.rbs
    Change code attribute type to integer                                       

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

    • Changed @code attribute type to integer.
    +3/-3     
    script.rbs
    Update script initialization and handler methods                 

    rb/sig/selenium/web_driver/script.rbs

  • Updated initialize method to use Remote::BiDiBridge.
  • Made block parameters optional in handler methods.
  • +4/-4     
    Documentation
    1 files
    struct.rb
    Update comment for clarity in BiDi module                               

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

    • Adjusted comment for clarity in the BiDi module.
    +1/-3     

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

    @aguspe aguspe changed the title Update types to reduce RBS errors to 0 [rb] Reduce RBS errors to 0 Nov 1, 2024
    @aguspe aguspe marked this pull request as ready for review November 1, 2024 22:52
    Copy link
    Contributor

    PR Reviewer Guide 🔍

    Here are some key observations to aid the review process:

    🎫 Ticket compliance analysis 🔶

    10943 - Partially compliant

    Fully compliant requirements:

    • Add rbs files to Ruby Selenium

    Not compliant requirements:

    • Improve IDE support for Ruby Selenium, especially with meta-programming
    • Enable type hints in IDEs like RubyMine
    ⏱️ Estimated effort to review: 4 🔵🔵🔵🔵⚪
    🧪 No relevant tests
    🔒 No security concerns identified
    ⚡ Recommended focus areas for review

    Type Checking Bypass
    The use of steep:ignore comments around the add_message_handler method may hide potential type-related issues. Consider if this is necessary or if the method can be refactored to comply with type checking.

    Type Checking Bypass
    The use of steep:ignore comments around the initialize method may hide potential type-related issues. Consider if this is necessary or if the method can be refactored to comply with type checking.

    Type Checking Bypass
    The use of steep:ignore comments around the new_window method may hide potential type-related issues. Consider if this is necessary or if the method can be refactored to comply with type checking.

    Type Checking Bypass
    The use of steep:ignore comments around the call method may hide potential type-related issues. Consider if this is necessary or if the method can be refactored to comply with type checking.

    Type Safety Concern
    The @options hash has been changed to use untyped keys and values. This may reduce type safety and make it harder to catch errors at compile-time. Consider using more specific types if possible.

    Copy link
    Contributor

    PR Code Suggestions ✨

    Explore these optional code suggestions:

    CategorySuggestion                                                                                                                                    Score
    Enhancement
    Specify more precise types for the @options hash to improve type safety and clarity

    Instead of using untyped for both keys and values in the @options hash, consider
    specifying more precise types if possible. This could improve type checking and code
    clarity.

    rb/sig/lib/selenium/webdriver/common/options.rbs [9]

    -@options: Hash[untyped, untyped]
    +@options: Hash[Symbol | String, String | Numeric | bool | nil]
    • Apply this suggestion
    Suggestion importance[1-10]: 7

    Why: The suggestion to specify more precise types for the @options hash enhances type safety and code clarity, aligning with good coding practices. However, it assumes specific types without context from the PR, which may not be accurate.

    7
    Specify a more accurate return type for the execute method to improve type safety

    Consider using a more specific return type for the execute method. If the method
    always returns a String, keep it as is. If it can return other types, consider using
    a union type or a more general type that accurately represents all possible return
    values.

    rb/sig/lib/selenium/webdriver/remote/bridge.rbs [253]

    -def execute: (untyped command, ?::Hash[untyped, untyped] opts, ?untyped? command_hash) -> String
    +def execute: (untyped command, ?::Hash[untyped, untyped] opts, ?untyped? command_hash) -> String | Hash[untyped, untyped] | nil
    • Apply this suggestion
    Suggestion importance[1-10]: 6

    Why: The suggestion to use a union type for the return value of the execute method could improve type safety if the method indeed returns multiple types. However, without context from the PR, it's speculative.

    6

    💡 Need additional feedback ? start a PR chat

    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.

    1 participant