-
-
Notifications
You must be signed in to change notification settings - Fork 8.2k
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
Add RBS files to Ruby #12844
Add RBS files to Ruby #12844
Conversation
Hey, how is it going with this? We don't have to do it all in one go if you don't want; it might be easier to ensure it is all rebased properly. Just mark it as out of draft when you're ready for us to pull something. I'm not familiar yet with these files, but I'm looking forward to using them. Thanks. |
Codecov ReportAttention:
❗ Your organization needs to install the Codecov GitHub app to enable full functionality. Additional details and impacted files@@ Coverage Diff @@
## trunk #12844 +/- ##
=======================================
Coverage 56.51% 56.51%
=======================================
Files 86 86
Lines 5255 5255
Branches 187 187
=======================================
Hits 2970 2970
Misses 2098 2098
Partials 187 187
☔ View full report in Codecov by Sentry. |
Hey, it's slowly but surely going, it would definitely be easier to review and test to make smaller PRs. I can wrap it up this weekend and prepare it for review and merge. For now, I created small commits that I will squash afterward just to make the development on the draft easy to follow. |
what library is needed to use it? |
I think steep is the best option, I added a local steep file that I did not commit yet since I'm still testing the config. I will add to the description of this PR and update the docs on how to use steep, but pretty much once that you have the config you just need to run 'steep check' and that's it |
So we can put it in the CI to run the check similar to what we do with rubocop and it will verify our test code? |
Exactly, we can add it, the same as a Rubocop check, I can add it as part of the rb actions flows (https://github.com/SeleniumHQ/selenium/blob/trunk/.github/workflows/ci-ruby.yml). However, I never worked directly on Bazel but I was just looking into that, I could modify the lint test to make it run with steep and validate the types. Also sorry for the delayed replies, I'm on CET time so I might always be 7 to 8 hours delayed haha. |
No worries, we can get the bazel pieces sorted later if needed, and this project is very geographically distributed, so communication delays are the norm. |
@titusfortner it's ready for review now, I added RBS and steep as dev dependencies and I would like to update the documentation, where would you suggest I update the documentation for other devs regarding type checking? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you for submitting this massive PR. I am not familiar with RBS/Steep, so maybe you can clarify few points for me?
- Should
sig
be shipped as part of the gem or pushed to https://github.com/ruby/gem_rbs_collection/? - I see plenty of
untyped
in definitions, should those be replaced with something more strict at some point? - Should be run type check for
spec/
folder as well? - There are some code duplications across RB/RBS files (e.g. URLs in
EDGE_COMMANDS
orDEFAULT_SECURE_SSL
). Is this expected? - There are comments in RBS, but they don't seem to provide any value and look like copies from RB files.
- How do we maintain
rb/rbs_collection.lock.yaml
?
Hello, sorry for the delay, it's busy week haha
So for example in https://github.com/ruby/gem_rbs_collection/blob/main/docs/CONTRIBUTING.md the recommendation is to ship the sig folder as part of the gem if you are the owner of it
If I wanted RBS to recognize a class or a module I had to create the file for that class or module to finish the RBS work in that, and it kept expanding more and more ( I could also do it in this PR, it will just take longer and it will probably get bigger)
For more info: https://github.com/ruby/rbs/blob/master/docs/collection.md#how-it-works So @p0deje during the weekend I will be updating point 4 & 5, and I would suggest to do the rest of the support as support for the spec folder and expanding the types in other PRs to limit the size and make it easier to review |
5a65b70
to
81a1670
Compare
@p0deje It's ready for a re-review now |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think it's a great start, we'd need to integrate it into specs/Bazel/CI at some point, but I agree it should be done in separate PRs.
…y actions, pointer actions, scroll origin
…ing and has_launching.rbs
wow, steep really pulls in all the things, doesn't it? @p0deje where is bazel supposed to be pulling this from?
|
@titusfortner It should be pulling from the internet, I wonder if our Gemfile.lock just needs to be properly updated. |
Hmm, @aguspe what is your local OS? Let me pull down a version of jruby and see what happens. 😁 |
Ok, I can reproduce this locally on my mac when I:
I get:
when I run it using RubyMine, everything is fine... |
My OS is Sonoma 14.0 and I'm using a mac M1, So I have been doing some research and testing it locally I can reproduce the same issue with jruby-9.4.3.0 & jruby-9.4.4.0 And even though I tried to force the installation of different versions of FFI, I still get 'Bundler::GemNotFound: Could not find ffi-1.16.0-java, ffi-1.16.0-java, ffi-1.16.0-java, ffi-1.16.0-java, ffi-1.16.0-java, ffi-1.16.0-java, ffi-1.16.0-java, ffi-1.16.0-java in locally installed gems' So I'm trying to debug this |
I've been there. @p0deje any ideas? Is there a way to just skip this for JRuby? 😁 |
So if we want to skip it for JRuby and option is to add this on selenium-webdriver.gemspec, as on the last commit: s.add_development_dependency 'steep', ['~> 1.5'] unless RUBY_PLATFORM == 'java' I tested this and it works for Jruby and the tests execute well locally at least |
Do we need to include it in gemspec at all? We could use it like that in Gemfile: gem 'rbs', '~> 3.2', require: false, platforms: %i[mri mingw x64_mingw]
gem 'steep', '~> 1.5', require: false, platforms: %i[mri mingw x64_mingw] This would allow installing it in MRI on Linux/Mac/Windows ignore both JRuby and TruffleRuby |
Here is the full patch that makes this work on MRI and JRuby: diff --git a/rb/Gemfile b/rb/Gemfile
index 9710e18d99..7b58c4e54f 100644
--- a/rb/Gemfile
+++ b/rb/Gemfile
@@ -6,3 +6,5 @@ Dir["#{__dir__}/*.gemspec"].each do |spec|
end
gem 'debug', '~> 1.7', require: false, platforms: %i[mri mingw x64_mingw]
+gem 'rbs', '~> 3.2', require: false, platforms: %i[mri mingw x64_mingw]
+gem 'steep', '~> 1.5.0', require: false, platforms: %i[mri mingw x64_mingw]
diff --git a/rb/Gemfile.lock b/rb/Gemfile.lock
index 5c72bfe7f7..bd8604143b 100644
--- a/rb/Gemfile.lock
+++ b/rb/Gemfile.lock
@@ -11,7 +11,8 @@ PATH
GEM
remote: https://rubygems.org/
specs:
- activesupport (7.1.1)
+ abbrev (0.1.1)
+ activesupport (7.1.2)
base64
bigdecimal
concurrent-ruby (~> 1.0, >= 1.0.2)
@@ -24,41 +25,44 @@ GEM
addressable (2.8.4)
public_suffix (>= 2.0.2, < 6.0)
ast (2.4.2)
- base64 (0.1.1)
+ base64 (0.2.0)
bigdecimal (3.1.4)
- bigdecimal (3.1.4-java)
concurrent-ruby (1.2.2)
connection_pool (2.4.1)
crack (0.4.5)
rexml
- csv (3.2.7)
+ csv (3.2.8)
debug (1.8.0)
irb (>= 1.5.0)
reline (>= 0.3.1)
diff-lcs (1.5.0)
- drb (2.1.1)
+ drb (2.2.0)
ruby2_keywords
ffi (1.16.3)
- fileutils (1.7.1)
+ ffi (1.16.3-x64-mingw32)
+ fileutils (1.7.2)
hashdiff (1.0.1)
i18n (1.14.1)
concurrent-ruby (~> 1.0)
io-console (0.6.0)
- irb (1.7.0)
- reline (>= 0.3.0)
+ irb (1.9.0)
+ rdoc
+ reline (>= 0.3.8)
json (2.6.3)
json (2.6.3-java)
language_server-protocol (3.17.0.3)
listen (3.8.0)
rb-fsevent (~> 0.10, >= 0.10.3)
rb-inotify (~> 0.9, >= 0.9.10)
- logger (1.5.3)
+ logger (1.6.0)
minitest (5.20.0)
- mutex_m (0.1.2)
+ mutex_m (0.2.0)
parallel (1.23.0)
parser (3.2.2.3)
ast (~> 2.4.1)
racc
+ psych (5.1.1.1)
+ stringio
public_suffix (5.0.1)
racc (1.7.1)
racc (1.7.1-java)
@@ -68,9 +72,12 @@ GEM
rb-fsevent (0.11.2)
rb-inotify (0.10.1)
ffi (~> 1.0)
- rbs (3.2.2)
+ rbs (3.3.0)
+ abbrev
+ rdoc (6.6.0)
+ psych (>= 4.0.0)
regexp_parser (2.8.1)
- reline (0.3.5)
+ reline (0.4.0)
io-console (~> 0.5)
rexml (3.2.5)
rspec (3.12.0)
@@ -113,7 +120,7 @@ GEM
ruby-progressbar (1.13.0)
ruby2_keywords (0.0.5)
rubyzip (2.3.2)
- securerandom (0.2.2)
+ securerandom (0.3.0)
steep (1.5.3)
activesupport (>= 5.1)
concurrent-ruby (>= 1.1.10)
@@ -129,8 +136,8 @@ GEM
securerandom (>= 0.1)
strscan (>= 1.0.0)
terminal-table (>= 2, < 4)
+ stringio (3.0.9)
strscan (3.0.7)
- strscan (3.0.7-java)
terminal-table (3.0.2)
unicode-display_width (>= 1.1.1, < 3)
tzinfo (2.0.6)
@@ -147,6 +154,8 @@ GEM
PLATFORMS
java
ruby
+ universal-java-1.8
+ universal-java-17
universal-java-18
x64-mingw32
@@ -154,14 +163,14 @@ DEPENDENCIES
debug (~> 1.7)
rack (~> 2.0)
rake (~> 13.0)
- rbs (~> 3.2.0)
+ rbs (~> 3.2)
rspec (~> 3.0)
rubocop (~> 1.42)
rubocop-performance (~> 1.15)
rubocop-rspec (~> 2.16)
selenium-devtools!
selenium-webdriver!
- steep (~> 1.5)
+ steep (~> 1.5.0)
webmock (~> 3.5)
webrick (~> 1.7)
yard (~> 0.9.11)
diff --git a/rb/selenium-webdriver.gemspec b/rb/selenium-webdriver.gemspec
index 42254af43d..35349a72a9 100644
--- a/rb/selenium-webdriver.gemspec
+++ b/rb/selenium-webdriver.gemspec
@@ -62,12 +62,4 @@ Gem::Specification.new do |s|
s.add_development_dependency 'webmock', ['~> 3.5']
s.add_development_dependency 'webrick', ['~> 1.7']
s.add_development_dependency 'yard', ['~> 0.9.11']
-
- if RUBY_PLATFORM == 'java'
- s.add_development_dependency 'rbs', ['1.2.0']
- s.add_development_dependency 'steep', ['0.46.0']
- else
- s.add_development_dependency 'rbs', ['~> 3.2.0']
- s.add_development_dependency 'steep', ['~> 1.5']
- end
end @aguspe I can merge your PR and commit this on top if you believe it's mergeable. |
Hooking this into Bazel is going to be non-trivial, I've already submitted ruby/rbs#1624 and soutaro/steep#962, but there is a lot of work still to do. This shouldn't stop us from merging the PR though! |
How does this get used with bazel? Isn't this just type hints to show up in IDE or are we looking for bazel to be able to enforce typing? |
@titusfortner I suppose we'd like to run typecheck on CI too to make sure we don't introduce invalid RBS sgnatures. |
I think it is, thank you @p0deje for looking into this, this is better than having a conditional on the gemspec |
@aguspe would you like to commit this or do you want me to? |
I added the suggested changes on the last commit, the only difference is that I added Steep from 1.6.0 and remove the need for RBS. Steep depends on RBS, so when we install Steep we already have access to RBS on the commandline, so there is no need to install it again |
I had to use Steep 1.5 due to soutaro/steep#962 |
Makes sense, reverted to 1.5.0 |
@titusfortner After this is merged, I will make a second PR to keep expanding RBS support |
@aguspe Thank you for the contribution! |
Description
I created RBS files for the following classes and modules so far:
Also, I added support for steep: https://github.com/soutaro/steep
You can run the following to do the type check:
steep check
This PR is the first one of multiple PRs adding support for RBS and type check to the ruby library.
Motivation and Context
Ruby Selenium does a lot of meta-programming which makes it more difficult to use with an IDE like RubyMine.
Adding rbs files, *should make this much easier.
More information and further discussion can be found here: #10943
Types of changes
Checklist