-
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
activerecord: Add types for create_with to singleton that inherited activerecord base #760
Conversation
@dak2 Thanks for your contribution! Please follow the instructions below for each change. Available commandsYou can use the following commands by commenting on this PR.
|
9d97243
to
599024e
Compare
@@ -1,4 +1,10 @@ | |||
module ActiveRecord | |||
class Base | |||
module ClassMethods[Model, Relation, PrimaryKey] | |||
def create_with: (nil | Hash[untyped, untyped]) -> Relation |
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.
Why do you add this to each version? It seems all definitions are the same. I feel it's enough to add this to activerecord.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.
Thanks for your comment!
I didn't know that if I write rbs in activerecord.rbs, rbs is shared in each version.
I rewrote create_with rbs in activerecord.rbs.
599024e
to
0cdedb4
Compare
@@ -51,6 +51,7 @@ module ActiveRecord | |||
def self.create!: (**untyped) -> instance | |||
def self.validate: (*untyped, ?if: condition[instance], ?unless: condition[instance], **untyped) -> void | |||
def self.validates: (*untyped, ?if: condition[instance], ?unless: condition[instance], **untyped) -> void | |||
def self.create_with: (nil | Hash[untyped, untyped]) -> Relation |
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.
Could you move this into ActiveRecord::Base::ClassMethods
, please?
And please copy it onto _ActiveRecord_Relation_ClassMethods
too.
The rbs_rails gem uses these components and effects the subclasses of ActiveRecord::Base.
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 missed both of them. I did it.
Thanks for your review!
…ctiverecord base Singleton that inherited activerecord base can handle create_with method. https://github.com/rails/rails/blob/6df15e25c875e9c1ee39acb0bc161b6c47531331/activerecord/test/cases/scoping/relation_scoping_test.rb#L499-L507 - create_with - https://api.rubyonrails.org/v6.0.6.1/classes/ActiveRecord/QueryMethods.html#method-i-create_with
0cdedb4
to
56a0e85
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
Thank you for your work!
/merge |
Singleton that inherited activerecord base can handle create_with method.
https://github.com/rails/rails/blob/6df15e25c875e9c1ee39acb0bc161b6c47531331/activerecord/test/cases/scoping/relation_scoping_test.rb#L499-L507