-
Notifications
You must be signed in to change notification settings - Fork 138
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
Modernize multi-sig generation in DSL compilers #1961
Conversation
93d0c6f
to
509864e
Compare
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 we also need to bump the rbi
version requirement to include the change, v0.1.10
EDIT: We also have #1960 which is bumping it btw
sigs = [] | ||
|
||
if !block || !parameters.empty? || return_type | ||
# If there is no block, and the params and return type have not been supplied, then |
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.
# If there is no block, and the params and return type have not been supplied, then | |
# If there is no block, and the params and return type have been supplied, then |
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.
Do you think we should document that multi-sigs will need to supply a block that adds sigs? It might be tough to figure out by looking at this method.
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.
Tbh, we haven't been documenting any of these methods and people have been using the patterns they see being used elsewhere in these files.
So, I don't know how to even start documenting this method and how it works.
RBI::BlockParam.new("block"), | ||
], | ||
) | ||
common_relation_methods_module.create_method(method_name.to_s) do |method| |
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.
Was this change from count
to method_name.to_s
intentional?
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.
Yes, it was, and I actually wanted to do it for other methods as well, but Copilot made many of the changes for me, so I lost steam.
The reason why I want to use method_name.to_s
is because it is better for future extension. If there is another method that behaves exactly as count
(let's say they add an alias for count
), then it will be enough to add that to the where
clause, since the method name is dynamic like this.
509864e
to
4ef2b0f
Compare
Labelling this as
breaking-change
since we are removing some methods that used to be publicly exported to DSL compilers.Motivation
Since Shopify/rbi#284 landed, we have access to a cleaner way to define methods with multiple signatures, but we'd been relying on an older way of doing it via a
create_method_with_sigs
helper method. This PR cleans this situation up.Implementation
create_method
to not define any signatures if ablock
is passed and theparameters
andreturn_type
have not been passed. This allows the block to define all the parameters and different return types explicitly.create_method_with_sigs
to use the newcreate_method
based mechanism to do multiple signature definitions.create_method_with_sigs
andcreate_sig
methods from the RBI extension methods.Tests
All existing tests should pass.