Skip to content
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 :finish_in_order option #339

Merged
merged 1 commit into from
Dec 14, 2023
Merged

Conversation

shaicoleman
Copy link
Contributor

See #338 for discussion

Comment on lines +650 to +652
options[:finish_items] ||= []
options[:finish_items_done] ||= []
options[:finish_index] ||= 0
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can these be local variables ?

Copy link
Contributor Author

@shaicoleman shaicoleman Dec 13, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You can't use local variables, as they need to maintain the state between invocations.
As they are being called via a class method, it can't use instance variables either.
The options are dup'ed so works even when invoked multiple times with the same options

Comment on lines +655 to +656
options[:finish].call(item, index, result)
options[:finish_index] += 1
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

can do this in the loop to not have the code twice ?

Copy link
Contributor Author

@shaicoleman shaicoleman Dec 13, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To do it in the loop, you would have to store the item first, e.g. something like this:
This is a bit more readable, but uses a tiny bit more memory.

    def instrument_finish_in_order(item, index, result, options)
      options[:mutex].synchronize do
        options[:finish_items] ||= []
        options[:finish_items_done] ||= []
        options[:finish_index] ||= 0
        options[:finish_items][index] = item
        options[:finish_items_done][index] = true
        break unless index == options[:finish_index]
        index.upto(options[:finish_items].size).each do |old_index|
          break unless options[:finish_items_done][old_index]
          old_item = options[:finish_items][old_index]
          options[:finish].call(old_item, old_index, result)
          options[:finish_index] += 1
        end
      end
    end

Let me know if you prefer this version and I'll update the PR

@grosser grosser merged commit 6cd55df into grosser:master Dec 14, 2023
7 checks passed
@grosser grosser mentioned this pull request Dec 14, 2023
@grosser
Copy link
Owner

grosser commented Dec 14, 2023

FYI refactored #340

@grosser
Copy link
Owner

grosser commented Dec 14, 2023

lmk if you can spot any more issues, otherwise I'll merge this and then release a new minor 🎉

@grosser
Copy link
Owner

grosser commented Dec 16, 2023

1.24.0

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants