-
Notifications
You must be signed in to change notification settings - Fork 109
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
Add reflection on views, fields, and associations #357
Conversation
Signed-off-by: Jordan Hollinger <[email protected]>
@ritikesh I hesitate to share the code as it's in a private, company repo atm. I do plan to open source it into procore-oss once all this is merged and I get company approval. The skeleton is in #358's description, but the magic is mostly in |
@jhollinger would you be able mimic that with a raw new rails demo app with a few models like posts/users/comments etc? Would just help visualise better is all :) |
@ritikesh Here's a gist with the relevant bits https://gist.github.com/jhollinger/9a9056b87c3bef1cb13c1032d3f3bf89. A trivial example of course. Real-world apps that benefit would have much broader and more deeply nested associations. |
thanks @jhollinger, this helps. one final question - the |
@ritikesh Correct, that's an ActiveRecord patch the gem with the extension will make. |
LGTM then. |
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.
Great work on this! Left some comments, but overall the direction LGTM!
lib/blueprinter/reflection.rb
Outdated
@view.included_view_names.reduce({}) do |acc, view_name| | ||
view = @blueprint.views.fetch(view_name) | ||
acc.merge view.send(type) | ||
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.
This makes me wonder if we could handle this higher level in Reflection
, and avoid calling back up it from this class. In other words, can we pass in dependent Reflection::View
instances on initialization.
Of course, this introduces an order dependency, which I don't believe we currently enforce.
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.
Is the concern that we're calling methods on Blueprinter::View
here? I guess we could pass view.name, view.fields, view.included_view_names
to 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.
It's more the circular dependency we're introducing here (e.g. Blueprinter#reflections
aggregates Reflection::View
objects, which can then call back to Blueprinter#reflections
). Happy to consider this a potential follow up, though!
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 if we initialized Reflection::View
s "bottom up" (leaf views first), that could work. I'll think on it a bit.
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.
@lessthanjacob I was able to re-use ViewCollection#fields_for
in Reflection::View
. It's quite a bit cleaner and should be easier to maintain as features are changed/added.
Co-authored-by: Jake Sheehy <[email protected]> Signed-off-by: Jordan Hollinger <[email protected]>
Signed-off-by: Jordan Hollinger <[email protected]>
Signed-off-by: Jordan Hollinger <[email protected]>
Signed-off-by: Jordan Hollinger <[email protected]>
Signed-off-by: Jordan Hollinger <[email protected]>
Signed-off-by: Jordan Hollinger <[email protected]>
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.
these seem like big changes, can you pls update version and add a changelog entry as well?
@ritikesh We've previously been releasing independently of any particular PR (at least for the past few versions). We can discuss further in Discord if we'd like to change this, though! |
Would love to see this gist again! |
@peterkarman1 Sorry, I was cleaning out some old gists. The docs for reflection & extensions are in #379. And watch this space: there may be some extensions in the works... |
@peterkarman1 Here's the extension that came from that gist https://github.com/procore-oss/blueprinter-activerecord |
Initial attempt at a reflection API per #341. I've built a POC ActiveRecord automagic preloader against this and #358.
Blueprinter's native views and associations have been wrapped in simpler structs or classes, as the native ones made it pretty hard to get at stuff. Could also protect against internal changes in the future.
I exposed the internal
method
andname
asname
anddisplay_name
respectively, since Rubocop (and best practices) don't allow methods named "method".TODO