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

generate overloaded sigs for active record sum #1830

Merged

Conversation

stathis-alexander
Copy link
Contributor

Motivation

Closes #1822. See discussion there for full context.

TL;DR: The current sigs are incorrect when given a block.

Tests

I believe I updated the spec here correctly.

@stathis-alexander
Copy link
Contributor Author

Is it possible to have someone add a label to and review this PR? I don't seem to be able to.

@bdewater
Copy link
Contributor

#1856 was merged, this needs a rebase.

@stathis-alexander
Copy link
Contributor Author

stathis-alexander commented Apr 11, 2024

@bdewater Thanks, I've rebased with main.

I'm not sure how to handle the conflict with the RBI gem now. I can rename this helper method, but it seems this helper should take type parameters as an optional argument.

@stathis-alexander stathis-alexander force-pushed the ajs/correct-sigs-for-active-record-sum branch from 379d0ed to 4cf0b2a Compare April 11, 2024 22:18
@bdewater
Copy link
Contributor

Tapioca monkey patches the rbi gem to add create_sig so this method ends up in sorbet/rbi/gems/rbi@0.1.10.rbi. In the current state the patch signature and the RBI file signature don't match and Sorbet complains. Running bundle exec tapioca gem rbi to force it to recompile the RBI file for rbi (the gem) should fix the issue 😄

@stathis-alexander stathis-alexander force-pushed the ajs/correct-sigs-for-active-record-sum branch 3 times, most recently from 841f887 to 7cecdc8 Compare April 12, 2024 15:45
@stathis-alexander
Copy link
Contributor Author

@bdewater - Thanks for this pointer. I didn't realize that tapioca would generate the sigs for the gem including the local monkey patches.

I've updated the RBI and fixed the tests.

@KaanOzkan
Copy link
Contributor

@stathis-alexander can you rebase on latest main?

@stathis-alexander stathis-alexander force-pushed the ajs/correct-sigs-for-active-record-sum branch from 7cecdc8 to 3ea0689 Compare April 15, 2024 18:42
@stathis-alexander
Copy link
Contributor Author

@KaanOzkan - Done!

@stathis-alexander stathis-alexander force-pushed the ajs/correct-sigs-for-active-record-sum branch from 3ea0689 to 93cb91e Compare April 15, 2024 18:55
@KaanOzkan KaanOzkan requested a review from egiurleo April 15, 2024 19:53
@egiurleo egiurleo merged commit bfc8731 into Shopify:main Apr 19, 2024
19 checks passed
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.

Active record relations compiler generates incorrect sum sigs
5 participants