-
Notifications
You must be signed in to change notification settings - Fork 76
feat: add polymorphic resource support #359
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
Conversation
|
Thanks for the PR. I’m on a trip for the next two weeks so unless someone else jumps in there will be a bit of a delay in looking this over. Just wanted to let you know I see the PR! |
mattpolzin
left a comment
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.
This looks like a pretty reasonable approach to me. I'll do a bit more thinking on it but in the meantime there are a few things I found to comment on.
ee95988 to
fa2d880
Compare
|
Hey there, |
|
Hey there :) |
|
Hey! Sorry, I kept not thinking of this PR when I had time to look at it because it had been removed from my review queue after my previous comments. I've added it back into my queue so I should get this reviewed in the next few (week) days. |
mattpolzin
left a comment
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.
This looks pretty good. I only have one inline comment.
I do have one high level question, though. Is it better (or more versatile) or just different to do it the way you've got it in this PR -- separate functions for type, fields, and relationships that take data and determine a result based on it -- versus having a mechanism by which a View can choose a more specific other View module so that you could implement your polymorphism via say 3 distinct JSONAPI.View modules (in the old style already supported by this library) and have one new JSONAPI.View module that had a single function that took the data and returned the module of the 3 possibilities that should be used for that data.
Hm I'd have to go over my approach vs the approach you described. Could you give me a short example on how that would look like? I can't seem to find any example for that in the docs, maybe it is maintainer knowledge? :P |
Sorry for the delay. I've been busy enough at work that I haven't and still don't have time to reply in detail. The suggestion was not to follow an existing pattern found elsewhere in this library, just a high level alternative idea. In broad strokes, you could have a callback like the following: @callback polymorphic_specialization(data()) :: module()As an alternative to several callbacks for polymorphic relationships, attributes, etc. Then you could define "regular" Regardless, don't worry if you don't have time to think on that approach. Instead, give my inline comment a reply and we can move forward with this PR because it looks like it does the trick and does not break anything and it's a very nice feature to have. |
|
Ahh now I get it, thanks for the clarifaction. Let me have a look, I like the idea of it! |
|
So I gave it a go but I am not really fluent in meta programming. |
mattpolzin
left a comment
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 thinking through some of this in response to my questions and comments. This looks good and it will be nice to have support for polymorphic resources in this way.
Hey there,
I tried my best to add support for polymorphic resources.
The idea is to allow the user to infer resource specifics from the given data.
Currently this only allows
typeandfieldsto be polymorphic. I will probably implement relationships as well.Additionally I only tested it to work as a relation with an api endpoint of its own - quite frankly there are some things that are not possible:
The
QueryParserfetchesview.fields()at a moment where no data is present, thus inference is not possible.This is my first time really working with a behaviour, my approach on implementing a backwards compatible solution felt a bit clunky, so any feedback is very welcome. :)