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

Proof of concept: Filter gem RBIs by version during annotations command #1585

Open
wants to merge 2 commits into
base: main
Choose a base branch
from

Conversation

egiurleo
Copy link
Contributor

@egiurleo egiurleo commented Jul 27, 2023

Motivation

This proof of concept aims to solve the problem of versioning RBIs. Up until now, there is no default way to write RBI for different versions of the same gem. This can lead to confusing issues for Sorbet users where RBIs don't match the version of a gem they're using, and they have to spend time figuring out whether their gem is out of date, whether the RBI is wrong, or if it's even both! Having a way to specify RBI for multiple versions of the same gem would allow people to adopt and use Sorbet without having to go through this hassle.

Implementation

The actual RBI filtering behavior is implemented in the RBI repo. This uses the FilterVersions rewriter during the annotations command to remove parts of the annotation RBI that aren't relevant to a certain gem version.

Tests

I have added a test for this functionality.

Further work

This is just a proof of concept to get a sense of how people are feeling before I invest more time in this. If I kept working on it, I would add:

  • Perhaps an experimental flag?
  • Config/CLI options to enable and disable this functionality?
  • Documentation

@egiurleo egiurleo force-pushed the emily/rbi-versioning branch 3 times, most recently from 0635c79 to b21d9bd Compare January 31, 2024 21:52
@egiurleo egiurleo changed the title [WIP] Filter gem RBIs by version during annotations command Proof of concept: Filter gem RBIs by version during annotations command Jan 31, 2024
@egiurleo egiurleo marked this pull request as ready for review February 2, 2024 21:46
@egiurleo egiurleo requested a review from a team as a code owner February 2, 2024 21:46
@egiurleo egiurleo requested review from andyw8 and Morriar and removed request for a team, andyw8 and Morriar February 2, 2024 21:46
@egiurleo egiurleo marked this pull request as draft February 2, 2024 21:48
@egiurleo egiurleo added the enhancement New feature or request label Feb 21, 2024
@egiurleo egiurleo marked this pull request as ready for review February 22, 2024 18:19
Copy link
Member

@paracycle paracycle left a comment

Choose a reason for hiding this comment

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

I have a few inline comments, but additionally:

Is this still a "Proof of concept"? I think once we merge this, it will ship in the next release, right?

Comment on lines +55 to +58
gem_names_and_versions = T.let(Hash.new { |h, k| h[k] = [] }, T::Hash[String, ::Gem::Version])
parser.specs.each_with_object(gem_names_and_versions) do |spec, hash|
hash[spec.name] = spec.version
end
Copy link
Member

Choose a reason for hiding this comment

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

I am finding it really hard to understand what this code is doing.

Is this something other than:

Suggested change
gem_names_and_versions = T.let(Hash.new { |h, k| h[k] = [] }, T::Hash[String, ::Gem::Version])
parser.specs.each_with_object(gem_names_and_versions) do |spec, hash|
hash[spec.name] = spec.version
end
gem_names_and_versions = parser.specs.to_h do |spec|
[spec.name, spec.version]
end

ensure
GitAttributes.create_vendored_attribute_file(@outpath)
end

sig { returns(T::Array[String]) }
sig { returns(T::Hash[String, ::Gem::Version]) }
Copy link
Member

Choose a reason for hiding this comment

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

Is there a special reason why we are reaching for a hash instead of a custom data structure?

If we were to build something like:

class GemInfo < T::Struct
  const :name, String
  const :version, ::Gem::Version
end

then, the changes in this PR will be more more minimal, and the code will be easier to read, IMO.

For example, this line will instead be:

Suggested change
sig { returns(T::Hash[String, ::Gem::Version]) }
sig { returns(T::Array[GemInfo]) }

and you would have preserved the return of an array.

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

Successfully merging this pull request may close these issues.

None yet

2 participants