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

Add a basic gem.rbi to lib/ruby. #92

Merged
merged 5 commits into from
Aug 7, 2019
Merged

Conversation

connorshea
Copy link
Contributor

The goal here is to resolve sorbet/sorbet#1455

There are a few problems with this as it is now:

  • Almost everything is untyped
  • Lots of _ parameters
  • This is specifically for RubyGems 3.x
  • I'm not sure if gem.rbi should be part of the ruby/ directory or not?
  • I'm not sure if gem.rbi should actually be rubygems.rbi?
  • Can Sorbet actually determine the user's RubyGems version?
  • There may be some minor errors, I generated this with Sord, but I've fixed a few of its quirks manually and generally I think it's been pretty well battle-tested at this point.

Adding this RBI to my sorbet/ directory and then regenerating hidden.rbi drops it down from 7419 lines to 3386 lines.

@connorshea connorshea requested a review from a team August 2, 2019 03:35
@ghost ghost requested review from elliottt and removed request for a team August 2, 2019 03:35
I'm stumped as to how the last one is supposed to be fixed.
@connorshea
Copy link
Contributor Author

Also, this exists already: https://github.com/sorbet/sorbet/blob/719eb639e535860a3f1e3288fed3dcd47586eb5e/rbi/stdlib/gem.rbi

So that's probably a problem

@connorshea
Copy link
Contributor Author

Any ideas on fixing this last error? 🤔

Connors-MacBook-Pro:sorbet-typed connorshea$ ruby .ci/run.rb 
lib/bundler/all/bundler.rbi:1211: Type Elem declared by parent T.class_of(Gem::Specification) must be re-declared in T.class_of(Bundler::EndpointSpecification) https://srb.help/5036
    1211 |class Bundler::EndpointSpecification < Gem::Specification
                ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
    https://github.com/sorbet/sorbet/tree/719eb639e535860a3f1e3288fed3dcd47586eb5e/rbi/stdlib/gem.rbi#L207: Elem declared in parent here
     207 |class Gem::Specification < Gem::BasicSpecification
                ^^^^^^^^^^^^^^^^^^
Errors: 1

@jez
Copy link
Collaborator

jez commented Aug 2, 2019

@connorshea I would propose removing the autogenerated sigs that have untyped. Putting them there locks in the sigs for everyone else, whereas by omitting them, people can one-off sig individual methods to have better types in the mean time while they're submitting a patch to add proper types upstream.

It'll also make the file smaller, so fewer lines in the sorbet/ folder.

@connorshea
Copy link
Contributor Author

Removed all the untyped stuff 👍

@elliottt
Copy link
Contributor

elliottt commented Aug 5, 2019

It looks like the error with the bundler rbi is due to the bundler.rbi that exists in sorbet https://github.com/sorbet/sorbet/blob/master/rbi/gems/bundler.rbi

@connorshea
Copy link
Contributor Author

Hmm, why do you think so?

@elliottt
Copy link
Contributor

elliottt commented Aug 6, 2019

The build failure for your most recent commit says that you need to declare Elem, but you're already doing that as of a86b6ec.

@connorshea
Copy link
Contributor Author

Any ideas on how to fix it?

AlmostNoSecurity = T.let(nil, T.untyped)
DIGEST_NAME = T.let(nil, T.untyped)
EXTENSIONS = T.let(nil, T.untyped)
H
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Did you mean to extend Enumerable here? That's likely what's causing problems with the bundler rbi.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

oh wow, good catch. I'm not entirely sure how that got in there.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You should also be able to remove the changes to the bundler rbi now.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

👍 done

Copy link
Contributor

@elliottt elliottt left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@elliottt elliottt merged commit 3cc976d into sorbet:master Aug 7, 2019
elliottt added a commit that referenced this pull request Aug 8, 2019
elliottt added a commit that referenced this pull request Aug 8, 2019
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.

Gem RBI
3 participants