-
Notifications
You must be signed in to change notification settings - Fork 1.4k
Fix relationship links doc #1981
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
Merged
Merged
Changes from all commits
Commits
File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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.
Isn't this true for all the adapters? It's just especially confusing for JSONAPI. Also, the name
links
here is arbitrary once you're mixing in the url helpers and just making an attribute return an custom object (hash).I'd also rather encourage usage of the serialization context over mixing in
include Rails.application.routes.url_helpers
but that's me. I sorta think using theinclude Rails.application.routes.url_helpers
should be a way of doing it until we unify the adapters or write better docs for serialization_context.Needs tests. That's part of the reason the doc fix is needed, right? we could put that off into a followup pr?
Uh oh!
There was an error while loading. Please reload this page.
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 is working for all the adapter but not correct for JSONAPI as the spec forbid to have an attribute named
links
:Also I had the url helper because within the
links
method there was a call toapi_v1_user_path
. And that call would not be correct without the url helper. I simply "fixed" the existing doc as I did not want to modify it to avoid having too many changes.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 suppose they should do it like ActiveModelSerializers::Adapter::JsonApi::Link and include ActiveModelSerializers::SerializationContext::UrlHelpers
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.
@bf4 Well I actually followed your own advices #1981 (comment). 😁 Now who should I listen to? @bf4 from the past or @bf4 from the present? 😕
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 dear god. I guess I... good eyes @groyoh