-
Notifications
You must be signed in to change notification settings - Fork 532
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
fix: allow multiple resource relation retrieval methods #1425
Conversation
@lgebhardt oh, cool, it's green. Shouldn't be too hard to add a failing test which this makes pass when I'm next at my computer |
93cea02
to
b4905b4
Compare
# frozen_string_literal: true | ||
|
||
module JSONAPI | ||
module RelationRetrieval |
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.
ideally, I think, this defines the required abstract interface, I think, required for a valid strategy
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! This looks good.
* fix: check if relation retrieval in included via included_modules * require 'jsonapi/relation_retrieval' * feat: raise when cannot include different retrieval strategy * test: multiple retrieval strategies --------- Co-authored-by: lgebhardt <[email protected]>
This PR was named from my experience that it wasn't letting me have a JSONAPI::Resource with the default retrieval method and also create another resource using the mixin with a different retrieval method.
While working on it, I realized it was possible to apply multiple different retrieval methods. If it's the same, it warns you, which I think is fine, but the new thing is that if it's different, it raises, since I can't imagine anything good could come from intentionally saying 'using v10' but it's already 'v09' and not knowing that 'v09' wasn't included
I saw some slightly different behavior in the JSONAPI::Resource vs. all manual includes so I'll try that in our app and see if it works by 100% avoiding JSONAPI::Resource
New Feature Submissions:
Bug fixes and Changes to Core Features:
Test Plan:
Reviewer Checklist: