toptal / chewy

High-level Elasticsearch Ruby framework based on the official elasticsearch-ruby client
MIT License
1.89k stars 368 forks source link

Fix return value of subscribed_task_stats #856

Closed fabiormoura closed 1 year ago

fabiormoura commented 2 years ago

The method subscribed_task_stats wraps almost all methods in RakeHelper but it has a bug.

For instance, the upgrade method in the same module is supposed to return a list of changed indexes in https://github.com/toptal/chewy/blob/7d567542c7685cf44a52050b8d16e61c373cd186/lib/chewy/rake_helper.rb#L65-L85

However, the wrapping method isn't returning to the caller the value of changed_indexes. Instead, it returns the result of the method output.puts which is nil.

I'm simply moving output.puts to be called within ensure so the output of the method correctly returns the result of:

ActiveSupport::Notifications.subscribed(Chewy::RakeHelper::JOURNAL_CALLBACK.curry[output], "apply_journal.chewy") do
    ActiveSupport::Notifications.subscribed(Chewy::RakeHelper::IMPORT_CALLBACK.curry[output], "import_objects.chewy", &block)
end

that would resolve to the correct value in the end when &block is evaluated.


Before submitting the PR make sure the following are checked:

konalegi commented 1 year ago

hey, @fabiormoura thanks for the fix, PR looks good, but, could you please provide a spec for the subscribed_task_stats method? Thanks!

konalegi commented 1 year ago

Also, please provide a changelog entry, so that will be easier to generate a release (example)

fabiormoura commented 1 year ago

Also, please provide a changelog entry, so that will be easier to generate a release (example)

Thanks for the suggestions @konalegi. I made the changes which have been amended.

konalegi commented 1 year ago

@fabiormoura could you please fix rubocop offenses as well? Thanks

fabiormoura commented 1 year ago

@fabiormoura could you please fix rubocop offenses as well? Thanks

it's been fixed.

konalegi commented 1 year ago

Merged, going to release it on Monday. Thanks for PR :)