-
Notifications
You must be signed in to change notification settings - Fork 109
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
activemodel: signature updates #739
activemodel: signature updates #739
Conversation
@hjwylde Thanks for your contribution! Please follow the instructions below for each change. Available commandsYou can use the following commands by commenting on this PR.
|
gems/activemodel/6.0/activemodel.rbs
Outdated
# person.errors[:name] # => ["cannot be nil"] | ||
# person.errors.delete(:name) # => ["cannot be nil"] | ||
# person.errors[:name] # => [] | ||
def delete: (untyped key, ?untyped type, **untyped) -> untyped |
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.
The type
and options
arguments were added in v6.1.
refs: rails/rails#32313
Therefore this change should be applied above 6.1 and above.
Please move this method to activermodel-6.0.rbs
from activemodel-generated.rbs
. After that, please add a new version of #delete
to activemodel/7.0/activemodel-7.0.rbs
and activemodel/7.1/activemodel-7.1.rbs
.
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.
Hmm, out of curiosity, how does this work with 6.1 then? Will 6.1 use the types from 6.0, and is this acceptable for now, or should a directory for 6.1 be added?
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.
6.x users will use the types for 6.0. So it's better to add a new type to the 6.1 directory. But Rails-6.1 is now outdated and not supported. Personally, it's not too late to add it after somebody wants it.
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.
Thanks for the check! I've fixed this up in the 6.0 file for now, I don't have an issue with this either, and if someone does need it then it can be added to 6.1 as you say.
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.
LGTM!
# Add a new type to the registry, allowing it to be gotten through ActiveModel::Type#lookup | ||
def self.register: (untyped type_name, ?untyped? klass, **untyped options) ?{ () -> untyped } -> untyped |
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.
👍
Note: The block parameter will be transferred to AM::Type::Registry#registry and processed in optional.
…ptions parameters for 7.0/7.1
a5a4451
to
03df0ff
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.
APPROVE
/merge |
Split from #735 to be smaller. Type signature updates for invalid signatures in activemodel, one is due to the block being marked as required, but it should be optional. The other (
delete
) is updated to match the method declaration (see https://api.rubyonrails.org/classes/ActiveModel/Errors.html#method-i-delete).