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

Custom enumerators #944

Merged
merged 1 commit into from
Jan 23, 2024
Merged

Custom enumerators #944

merged 1 commit into from
Jan 23, 2024

Conversation

rianmcguire
Copy link
Contributor

This is based on the work in #326 and the design suggested by @adrianna-chang-shopify in this comment.

It allows tasks to define their own custom enumerators, in order to support use cases like iteration over external API resources, or alternative database querying strategies.

It does this by exposing an overridable enumerator_builder method on Task, whose default implementation is the existing collection type inspection logic.

Fixes #207

def enumerator_builder(cursor:)
collection = self.collection

job_iteration_builder = JobIteration::EnumeratorBuilder.new(nil)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

It's a bit icky to pass in nil here - it's for the job argument.

@job is only referenced by EnumeratorBuilder#build_throttle_enumerator, which isn't called here, so there's no immediate issue with it being nil.

Alternatively, job could be provided as an additional argument to enumerator_builder, but leaking the job instance into the Task interface feels worse than passing in nil.

@rianmcguire
Copy link
Contributor Author

I have signed the CLA!

README.md Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
README.md Show resolved Hide resolved
Copy link
Contributor

@adrianna-chang-shopify adrianna-chang-shopify left a comment

Choose a reason for hiding this comment

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

This makes sense to me. The API seems straightforward and the example in the README is clear 👍 Could we add @sambostock as a co-author since some of his original contributions were ported over to this PR?

cc @Shopify/rails for review on this please.

README.md Outdated Show resolved Hide resolved
@gmcgibbon
Copy link
Member

Please squash your commits and let's give Sam co-author please. Thank you!

Co-authored-by: Sam Bostock <sam.bostock@shopify.com>
Co-authored-by: Gannon McGibbon <gannon@hey.com>
@rianmcguire
Copy link
Contributor Author

Please squash your commits and let's give Sam co-author please. Thank you!

Done! Thanks for the feedback and reviews everyone.

@gmcgibbon gmcgibbon merged commit 7c9e747 into Shopify:main Jan 23, 2024
21 checks passed
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.

Provide support for enumerable collections beyond Array and ActiveRecord::Relation in TaskJob
4 participants