-
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
rename context to serialization_context #1313
Conversation
LGTM 👍 |
@@ -0,0 +1,12 @@ | |||
module ActionController |
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 not sure the Context
class belongs in ActionController
, as we might want to use it for non-AC related stuff as well. Some mixin/concern definitely belongs here, but I think the base Context
class should be part of the ActiveModel(::)Serializer(s)
namespace. Thoughts?
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 agree - action controller isn't ever going to really make use of the context, where as it is AMS that utilizes context.
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 action controller only creates the context, and that's 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.
It's not in action_controller, it's namespaced within actioncontroller::serialization which is fine by me
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.
Alternative: lib/active_model_serializers/serialization_context.rb
module ActiveModelSerializers
class SerializationContext
#etc
end
end
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.
speaking for myself, I would mere that in the next 10 minutes
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.
Yeah, let's put it in active_model_serializers/serialization_context
and I'm OK to merge.
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.
@tchak amend commit, force push, and we'll merge
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.
@tchak want one of us to do it?
9214fc8
to
c0c4af8
Compare
c0c4af8
to
31172b1
Compare
@bf4 fixed and rebased. Sorry for the delay! |
Looks good to me. We are still in disarray about namespacing, but that's outside the scope of this. good work! |
rename context to serialization_context
A resubmit of my earlier PR. Removed all references to
url_helpers
for now. See #1289 (comment) for referencecc @bf4