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

Check duplicates between shims and Sorbet's RBI payload #879

Merged
merged 7 commits into from Apr 7, 2022

Conversation

Morriar
Copy link
Collaborator

@Morriar Morriar commented Apr 1, 2022

Motivation

Check duplicates between shims files and Sorbet's payload.

It's frequent that we locally fix Sorbet's payload in our shims waiting for the same patch to be merged upstream. The problem is that after a while we forget the local fixes and they stay there for ever. This can even become dangerous when the definition in the payload is later updated and we keep the shadowing local definition.

This PR introduces a way to check for duplicates between the local shims and Sorbet's payload.

Implementation

This feature uses Sorbet's --print=payload_sources to dump the RBIs from its payload so we can index them in the same way we do for gems RBIs and DSL RBIs.

I gated the feature behind a specific Sorbet version like we did for to_ary_nil_support.

Tests

See automated tests.

@Morriar Morriar added the feature label Apr 1, 2022
@Morriar Morriar requested a review from Apr 1, 2022
@Morriar Morriar self-assigned this Apr 1, 2022
@Morriar Morriar changed the title Group check rbi payload Check duplicates between shims and Sorbet's RBI payload Apr 1, 2022
@Morriar Morriar force-pushed the group-check-rbi-payload branch 2 times, most recently from 7dcecf8 to 33d04d9 Compare Apr 1, 2022
sig { returns(T::Boolean) }
def default_gem?; end
Copy link
Collaborator Author

@Morriar Morriar Apr 1, 2022

Choose a reason for hiding this comment

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

Copy link
Member

@vinistock vinistock left a comment

Very nice!

lib/tapioca/cli.rb Outdated Show resolved Hide resolved
lib/tapioca/helpers/sorbet_helper.rb Outdated Show resolved Hide resolved
@Morriar Morriar force-pushed the group-check-rbi-payload branch 2 times, most recently from 6342c0d to 30f8376 Compare Apr 5, 2022
Morriar and others added 7 commits Apr 5, 2022
Signed-off-by: Alexandre Terrasa <alexandre.terrasa@shopify.com>
Co-authored-by: Kaan Ozkan <kaan.ozkan@shopify.com>
Co-authored-by: Vinicius Stock <vinicius.stock@shopify.com>
Extracted in this commit to avoid a bigger, harder to read change in the next one.
No functional change here.

Signed-off-by: Alexandre Terrasa <alexandre.terrasa@shopify.com>
Signed-off-by: Alexandre Terrasa <alexandre.terrasa@shopify.com>
Signed-off-by: Alexandre Terrasa <alexandre.terrasa@shopify.com>
Dumping, parsing and indexing Sorbet's payload takes around ~5s.
We don't need to waste this time in most tests here.

Signed-off-by: Alexandre Terrasa <alexandre.terrasa@shopify.com>
This definition is duplicated from https://github.com/sorbet/sorbet/blob/master/rbi/stdlib/bundler.rbi#L9958.

Signed-off-by: Alexandre Terrasa <alexandre.terrasa@shopify.com>
Signed-off-by: Alexandre Terrasa <alexandre.terrasa@shopify.com>
@Morriar Morriar force-pushed the group-check-rbi-payload branch from 30f8376 to 91dcde9 Compare Apr 5, 2022
@Morriar Morriar requested a review from paracycle Apr 5, 2022
@Morriar Morriar merged commit cdaf17a into main Apr 7, 2022
9 checks passed
@Morriar Morriar deleted the group-check-rbi-payload branch Apr 7, 2022
@shopify-shipit shopify-shipit bot temporarily deployed to production May 13, 2022 Inactive
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants