Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
feat(iter): Added context accepting variants of Map & ForEach #114
base: main
Are you sure you want to change the base?
feat(iter): Added context accepting variants of Map & ForEach #114
Changes from 11 commits
60f3996
a22d62e
57ae9ca
722e19d
e9a88bf
c6581d3
4a15de5
3ae45b6
11cbc39
1d04c68
d71cc05
2f570b7
bba9127
825d1df
296e00b
9018777
08c699d
56a8612
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I would prefer to not add a dependency on
pool
in theiter
package. Because the iter package knows the number of inputs and the number of outputs in advance, it can be considerably more efficient than thepool
package, which must work for an unbounded number of iterations.I think it would be good to reconcile this PR with the patterns in #104. In particular, the
FailFast
flag should mean similar things iterating withForEachErr
andForEachCtx
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The abstractions built into
ContextPool
offered a simple way to achieve the functionality I was looking for with regard to context cancellation without having to duplicate code. Bothiter
andpool
usedwaitgroup
underneath. I'm not too sure what efficiency gains are made by having two separate implementations but I'd be happy to switch it back to original setup.Additionally, as a further argument for using
ContextPool
, theFailFast
behavior seems like it could be easily implemented by using the new bool to optionally callWithFirstError()
on the underlying pool. Right now I call that by default assuming the caller is only interested in the first error when using a context.I could add the *Err variants as extensions to what I have here that just return an error but don't require a context.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Related: #118
You might be right. The simplicity of using a
Pool
to back theiter
package is likely more valuable than the minor efficiency gains we get from knowing the size of the set in advance (which basically boil down to allocating in advance and not needing a mutex).Let me noodle on the design a bit and get back to ya. I'll probably open a draft that unifies this with #104 and the
pool
package in general, maybe just replacing theIterator
structs with configured pools.