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 all the RBS files to add full rbs support to all selenium rb classes and modules #13234

Merged
merged 82 commits into from
Mar 18, 2024

Conversation

aguspe
Copy link
Contributor

@aguspe aguspe commented Dec 2, 2023

Description

This PR adds more than 120 RBS files and updates the steep file to reduce all the current steep errors.
The goal with this PR is that afterward it would be possible to add a steep check on the bazel pipeline.

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

I stated on #13192 that I wanted to make smaller PRs to expand support and make it easier to review, but since there was a lot of work to be done to cover all the classes and modules in the library I decided to expand this PR to add all the missing coverage and fix all the steep issues

Right now if a steep check is run after the two previously mentioned PRs this is the result:

Screenshot 2023-12-02 at 20 31 30

Already with the files added and the extension of libraries on the steep config file the amount of errors drops to:

Screenshot 2023-12-02 at 19 57 48

This is currently a work in progress and I will submit this PR after all the type errors are reduced to 0 and all the files are properly updated to reflect the right types used in classes and not just undefined

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.

@aguspe
Copy link
Contributor Author

aguspe commented Mar 13, 2024

Thank you for the quick review and the help @p0deje, here in Denmark is quite late now, so I will check all the answers tomorrow morning

@aguspe
Copy link
Contributor Author

aguspe commented Mar 16, 2024

I made one more commit to remove all the comments created during auto generation, and cast the generic type to the constants that were mapped to a specific string or symbol during autogeneration, also I added some refactoring in general

After the last steep check, there are still no errors, and from there we can start replacing the untyped values for the right values and correct each error one at the time

@p0deje

Screenshot 2024-03-17 at 00 33 39

@p0deje
Copy link
Member

p0deje commented Mar 17, 2024

Can you please fix RuboCop complaints?

Inspecting 276 files
....................................................................................................................................CC...............C..............................................................................................................................

Offenses:

rb/lib/selenium/webdriver/firefox/profile.rb:27:27: C: [Correctable] Style/MutableConstant: Freeze mutable objects assigned to constants.
        WEBDRIVER_PREFS = { ...
                          ^
rb/lib/selenium/webdriver/firefox/profile.rb:28:11: C: [Corrected] Style/HashSyntax: Use the new Ruby 1.9 hash syntax.
          :port => 'webdriver_firefox_port',
          ^^^^^^^^
rb/lib/selenium/webdriver/firefox/profile.rb:29:11: C: [Corrected] Style/HashSyntax: Use the new Ruby 1.9 hash syntax.
          :log_file => 'webdriver.log.file'
          ^^^^^^^^^^^^
rb/lib/selenium/webdriver/firefox/profile.rb:169:37: C: [Corrected] Layout/SpaceInsideHashLiteralBraces: Space inside { detected.
            WebDriver.logger.debug({ extension: name }.inspect, id: :firefox_profile)
                                    ^
rb/lib/selenium/webdriver/firefox/profile.rb:169:53: C: [Corrected] Layout/SpaceInsideHashLiteralBraces: Space inside } detected.
            WebDriver.logger.debug({ extension: name }.inspect, id: :firefox_profile)
                                                    ^
rb/lib/selenium/webdriver/firefox/profiles_ini.rb:44:9: C: Metrics/CyclomaticComplexity: Cyclomatic complexity for parse is too high. [11/9]
        def parse ...
        ^^^^^^^^^
rb/lib/selenium/webdriver/remote/http/default.rb:70:11: C: Metrics/AbcSize: Assignment Branch Condition size for request is too high. [<5, 29, 8> 30.5/30]
          def request(verb, url, headers, payload, redirects = 0) ...
          ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

@aguspe
Copy link
Contributor Author

aguspe commented Mar 17, 2024

Can you please fix RuboCop complaints?

Inspecting 276 files
....................................................................................................................................CC...............C..............................................................................................................................

Offenses:

rb/lib/selenium/webdriver/firefox/profile.rb:27:27: C: [Correctable] Style/MutableConstant: Freeze mutable objects assigned to constants.
        WEBDRIVER_PREFS = { ...
                          ^
rb/lib/selenium/webdriver/firefox/profile.rb:28:11: C: [Corrected] Style/HashSyntax: Use the new Ruby 1.9 hash syntax.
          :port => 'webdriver_firefox_port',
          ^^^^^^^^
rb/lib/selenium/webdriver/firefox/profile.rb:29:11: C: [Corrected] Style/HashSyntax: Use the new Ruby 1.9 hash syntax.
          :log_file => 'webdriver.log.file'
          ^^^^^^^^^^^^
rb/lib/selenium/webdriver/firefox/profile.rb:169:37: C: [Corrected] Layout/SpaceInsideHashLiteralBraces: Space inside { detected.
            WebDriver.logger.debug({ extension: name }.inspect, id: :firefox_profile)
                                    ^
rb/lib/selenium/webdriver/firefox/profile.rb:169:53: C: [Corrected] Layout/SpaceInsideHashLiteralBraces: Space inside } detected.
            WebDriver.logger.debug({ extension: name }.inspect, id: :firefox_profile)
                                                    ^
rb/lib/selenium/webdriver/firefox/profiles_ini.rb:44:9: C: Metrics/CyclomaticComplexity: Cyclomatic complexity for parse is too high. [11/9]
        def parse ...
        ^^^^^^^^^
rb/lib/selenium/webdriver/remote/http/default.rb:70:11: C: Metrics/AbcSize: Assignment Branch Condition size for request is too high. [<5, 29, 8> 30.5/30]
          def request(verb, url, headers, payload, redirects = 0) ...
          ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^

Oh sorry for that, I forgot to run rubocop locally, my bad

Here is the latest rubocop and steep check run all with 0 issues, I added a rubocop ignore on the steep file because the steep config file doesn't follow rubocop recommended block size

Screenshot 2024-03-17 at 17 56 18 Screenshot 2024-03-17 at 17 56 05

@p0deje
Copy link
Member

p0deje commented Mar 18, 2024

Out of curiosity, what are these warnings?

[#type_check_file(lib/selenium/webdriver/remote/http/common.rb@lib)] [synthesize:(20:1)] [synthesize:(21:3)] [synthesize:(22:5)] [synthesize:(23:7)] [synthesize:(24:9)] [synthesize:(25:11)] [synth
esize: (70:11)] [synthesize:(70:23)] Unknown variable: (restarg)

@p0deje
Copy link
Member

p0deje commented Mar 18, 2024

Would it be okay if I squash your PR into a single commit?

@aguspe
Copy link
Contributor Author

aguspe commented Mar 18, 2024

Would it be okay if I squash your PR into a single commit?

For sure, that would be perfect, I usually make a lot of small commits to show progress and then I squash everything into one nice commit

@aguspe
Copy link
Contributor Author

aguspe commented Mar 18, 2024

Out of curiosity, what are these warnings?

[#type_check_file(lib/selenium/webdriver/remote/http/common.rb@lib)] [synthesize:(20:1)] [synthesize:(21:3)] [synthesize:(22:5)] [synthesize:(23:7)] [synthesize:(24:9)] [synthesize:(25:11)] [synth
esize: (70:11)] [synthesize:(70:23)] Unknown variable: (restarg)

From what I researched these errors are related to the splat operator that throws the restarg (unknown variable issue)

I will create this as a separate issue, but before it used to throw an actual error, now it just seems like a warning because of steep itself

soutaro/steep#424

soutaro/steep#7

@p0deje
Copy link
Member

p0deje commented Mar 18, 2024

Thank you again for this massive effort, I'll merge the PR as soon as CI passes.

Out of curiosity, did you use https://github.com/ruby/typeprof or something else to generate RBS files?

@aguspe
Copy link
Contributor Author

aguspe commented Mar 18, 2024

https://github.com/ruby/typeprof

I used RBS prototype: https://github.com/ruby/rbs (This is what rubymine does in the background)

An end user of rbs will probably find rbs prototype the most useful. This command generates boilerplate signature declarations for ruby files. For example, say you have written the below ruby script.

person.rb

class Person
  attr_reader :name
  attr_reader :contacts

  def initialize(name:)
    @name = name
    @contacts = []
  end

  def speak
    "I'm #{@name} and I love Ruby!"
  end
end

Rubymine documentation:

https://blog.jetbrains.com/ruby/2021/09/working-with-rbs-in-rubymine/
https://blog.jetbrains.com/ruby/2021/09/rbs-how-to-get-the-most-out-of-rubymine-code-assistance/

And I was thinking to keep working on this to add proper types now that all the files are there so I can learn more about the selenium ruby library if that is something that will be helpful @p0deje

@p0deje p0deje merged commit 5b7c95b into SeleniumHQ:trunk Mar 18, 2024
30 checks passed
@p0deje
Copy link
Member

p0deje commented Mar 18, 2024

And I was thinking to keep working on this to add proper types now that all the files are there so I can learn more about the selenium ruby library if that is something that will be helpful

That would be amazing. I'm curious if you could try typeprof as it might generate more precise signatures, not just boilerplate as rbs. And maybe you won't have to work on manually changing every signature.

@aguspe
Copy link
Contributor Author

aguspe commented Mar 18, 2024

And I was thinking to keep working on this to add proper types now that all the files are there so I can learn more about the selenium ruby library if that is something that will be helpful

That would be amazing. I'm curious if you could try typeprof as it might generate more precise signatures, not just boilerplate as rbs. And maybe you won't have to work on manually changing every signature.

I will give it a try to that, thank you for the suggestion 👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants