Conversation
lib/chewy/index/crutch.rb
Outdated
| execution_block = @index._crutches[:"#{name}"] | ||
| @crutches_instances[name] ||= if execution_block.arity == 2 | ||
| @crutches_instances[name] ||= if execution_block.arity == 3 | ||
| execution_block.call(@collection, self, options) |
There was a problem hiding this comment.
Why are we passing options here? We should be passing @update_fields right?
There was a problem hiding this comment.
options is a hash, which contains update_fields key. If we decise in the future to add more params to the crutch, we won't need to change arity, just add another key to options.
There was a problem hiding this comment.
Pull Request Overview
This PR adds support for passing an update_fields parameter through the BulkBuilder into the Crutches mechanism to allow crutch functions to receive a third argument when needed.
- BulkBuilder now initializes
Crutcheswithupdate_fields: @fields. Crutchesis updated to acceptupdate_fieldsin its initializer and dispatch blocks with arity 3.
Reviewed Changes
Copilot reviewed 2 out of 2 changed files in this pull request and generated 1 comment.
| File | Description |
|---|---|
| lib/chewy/index/import/bulk_builder.rb | Pass update_fields: @fields when constructing Crutches |
| lib/chewy/index/crutch.rb | Extend Crutches initializer and [] method to accept and forward update_fields |
Comments suppressed due to low confidence (2)
lib/chewy/index/crutch.rb:29
- Defining
update_fieldsas a method parameter shadows the instance variable set ininitializeand requires callers to supply it on every lookup. Consider removing the parameter here and using@update_fieldsfrom the initializer to keep the API simpler and consistent.
def [](name, update_fields = [])
lib/chewy/index/crutch.rb:31
- [nitpick] Memoizing crutch instances by
namealone may lead to stale instances ifupdate_fieldsvary. Consider incorporatingupdate_fieldsinto the cache key or ensuring the instance-level@update_fieldsis consistently applied.
if execution_block.arity == 3
|
|
||
| def crutches_for_index | ||
| @crutches_for_index ||= Chewy::Index::Crutch::Crutches.new @index, @to_index | ||
| @crutches_for_index ||= Chewy::Index::Crutch::Crutches.new @index, @to_index, update_fields: @fields |
There was a problem hiding this comment.
Should we update other places too, which have crutches.new, and if they have fields available, to pass it from there?
Added possibility to provide
update_fieldsparam to be passed to crutch function.update_fieldsis added on Crutches level during initialization in BulkBuilder for further usage inAbstractZpChewyIndex.traced_crutch(added to args for crutch invocation)Before submitting the PR make sure the following are checked:
[Fix #issue-number](if the related issue exists).master(if not - rebase it).