-
Notifications
You must be signed in to change notification settings - Fork 1.4k
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
Block syntax for associations. #1311
Conversation
@@ -29,7 +28,8 @@ class << base | |||
|
|||
module ClassMethods | |||
def inherited(base) | |||
base._reflections = self._reflections.try(:dup) || [] |
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 was this a try
before?
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.
Because the self._reflections
was not initialized in the constructor.
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.
so any class that inherits Associations
must have a _reflections
accessor?
can you document that, please?
I know it's outside the scope of a refactor, but since we have next to none of this type of documentation, I think it will be good to do it as we go.
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.
No, including Associations
does declare a _reflections
accessor.
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.
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.
oh ok, so you're setting base._reflections
just to an empty array?
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.
depending if the parent class has some reflections already or not
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'm confused. Sorry. Can you summarize?
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.
Module Associations
defines (on the class that includes it) a class attribute _reflections
that is initialized to []
and is inherited.
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.
To be clear, those changes are mainly (if not only) cosmetic, in order to be consistent with how we deal with inheritance of class attributes all around, they're not necessary for this PR.
@@ -17,7 +17,21 @@ class Serializer | |||
# | |||
# So you can inspect reflections in your Adapters. |
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.
what is a reflection?
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.
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.
oh. ha. didn't see there was more. thanks
2010f83
to
810d09b
Compare
Should be green. |
class << base | ||
attr_accessor :_reflections | ||
end | ||
base.class_attribute :_reflections |
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 don't think we want this to be instance readable/writable? Do we want these to be inheritable?
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.
re: inhertiable, yes, oops
- base._reflections = self._reflections.try(:dup) || []
+ super
+ base._reflections = _reflections.dup
Closed by #1356 |
This PR is half of #1262, introducing the block syntax for overriding associations (and removing the legacy method defining syntax).
Currently 2 tests fail, I believe those tests are ill (they seem to expect an association to be treated as an attribute).