Allow associations to be defined with a block #106
Conversation
| else | ||
| options.merge(extractor: AutoExtractor) | ||
| options.merge(extractor: AutoExtractor.new) |
There was a problem hiding this comment.
Since you're already instantiating these Extractor classes, do you mind also instantiating the Extractor in fields and identifier
here https://github.com/procore/blueprinter/blob/master/lib/blueprinter/base.rb#L41
and here https://github.com/procore/blueprinter/blob/master/lib/blueprinter/base.rb#L219
and then remove this https://github.com/procore/blueprinter/blob/master/lib/blueprinter/extractor.rb#L11-L13
There was a problem hiding this comment.
Removing https://github.com/procore/blueprinter/blob/master/lib/blueprinter/extractor.rb#L11-L13
makes this break: https://github.com/procore/blueprinter/blob/master/spec/integrations/shared/base_render_examples.rb#L41
This may be a breaking change, so I'm not sure if we want to change this here. Maybe I should just apply the other changes you mentioned but leave self.extract?
There was a problem hiding this comment.
I didn't realize there were tests for that. OK let's leave that for now. Thanks!
lib/blueprinter/base.rb
Outdated
| self, | ||
| options.merge(association: true)) | ||
| name, | ||
| AssociationExtractor.new(options[:extractor]), |
There was a problem hiding this comment.
I don't think you need to pass the options[:extractor] here, the AssociationExtractor should already be passed the options from Field https://github.com/procore/blueprinter/blame/master/lib/blueprinter/field.rb#L13
| def extract(association_name, object, local_options, options={}) | ||
| value = object.public_send(association_name) | ||
| return (value || options[:default]) if value.nil? | ||
| value = @field_extractor.extract(association_name, object, local_options, options) |
There was a problem hiding this comment.
you should be able to do options[:extractor].extract( .... )
|
Would you also add this feature to the README and add the method documentation above |
hugopeixoto
force-pushed
the
feature/pass-a-block-to-an-association
branch
from
14218f7
to
5073953
Compare
philipqnguyen
requested review from
dlcarter,
mcclayton,
njbbaer,
adamwells and
wai-procore
| { | ||
| "uuid": "733f0758-8f21-4719-875f-262c3ec743af", | ||
| "projects": [ | ||
| {"uuid": "b426a1e6-ac41-45ab-bfef-970b9a0b4289", "name": "query-console"}, |
There was a problem hiding this comment.
I see you've seen my library 😂
|
released with 0.7.0 |
Addresses #90.
I removed an extra hash from
BlockExtractor. Not sure if should have, but it seemed to be redundant, and tests are passing.I changed
AssociationExtractorso that it accepts a nested extractor. This allowed me to reuseAutoExtractorandBlockExtractor, making#associationnow very similar to#field.I noticed that we're passing classes as extractors, and that the class method
extractalways creates a new instance of itself. This will probably create an unnecessary number of objects. Instantiating them directly in#fieldand#associationmight help improve performance. I addressed that in this PR as well.