-
Notifications
You must be signed in to change notification settings - Fork 321
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
iter-err: add ForEachErr and ForEachIdxErr #104
base: main
Are you sure you want to change the base?
Conversation
Fixed linter issues |
Codecov Report
@@ Coverage Diff @@
## main #104 +/- ##
==========================================
+ Coverage 99.30% 99.36% +0.05%
==========================================
Files 12 12
Lines 433 474 +41
==========================================
+ Hits 430 471 +41
Misses 3 3
📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more |
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.
This is great work! A couple small comments inline.
iter/iter.go
Outdated
task := func() { | ||
i := int(idx.Add(1) - 1) | ||
for ; i < numInput && !failed.Load(); i = int(idx.Add(1) - 1) { | ||
if err := f(i, &input[i]); err != nil { | ||
errsMu.Lock() | ||
errs = multierror.Join(errs, err) | ||
errsMu.Unlock() | ||
|
||
failed.Store(iter.FailFast) | ||
} | ||
} | ||
} |
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.
It looks to me like FailFast does not guarantee returning only the error that caused the failure. One task can check if failed was set, error, and append its error while another task concurrently failed and appends an error.
I think this counts as surprising behavior given that the rest of the library guarantees that only the first error is retained when WithFirstError() is used.
I think we can fix this by checking the old value of failed
:
task := func() { | |
i := int(idx.Add(1) - 1) | |
for ; i < numInput && !failed.Load(); i = int(idx.Add(1) - 1) { | |
if err := f(i, &input[i]); err != nil { | |
errsMu.Lock() | |
errs = multierror.Join(errs, err) | |
errsMu.Unlock() | |
failed.Store(iter.FailFast) | |
} | |
} | |
} | |
task := func() { | |
i := int(idx.Add(1) - 1) | |
for ; i < numInput && !failed.Load(); i = int(idx.Add(1) - 1) { | |
if err := f(i, &input[i]); err != nil { | |
if alreadyFailedFast := failed.Swap(iter.FailFast); !alreadyFailedFast { | |
errsMu.Lock() | |
errs = multierror.Join(errs, err) | |
errsMu.Unlock() | |
} | |
} | |
} | |
} |
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.
fixed 👍
iter/iter_test.go
Outdated
t.Run("mutating inputs is fine", func(t *testing.T) { | ||
t.Parallel() | ||
ints := []int{1, 2, 3, 4, 5} | ||
_ = forEach(ints, func(val *int) error { | ||
*val += 1 | ||
return nil | ||
}) | ||
require.Equal(t, []int{2, 3, 4, 5, 6}, ints) | ||
}) | ||
|
||
t.Run("mutating inputs is fine", func(t *testing.T) { | ||
t.Parallel() | ||
ints := []int{1, 2, 3, 4, 5} | ||
_ = forEach(ints, func(val *int) error { | ||
*val += 1 | ||
return nil | ||
}) | ||
require.Equal(t, []int{2, 3, 4, 5, 6}, ints) | ||
}) |
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.
Looks like a duplicate
t.Run("mutating inputs is fine", func(t *testing.T) { | |
t.Parallel() | |
ints := []int{1, 2, 3, 4, 5} | |
_ = forEach(ints, func(val *int) error { | |
*val += 1 | |
return nil | |
}) | |
require.Equal(t, []int{2, 3, 4, 5, 6}, ints) | |
}) | |
t.Run("mutating inputs is fine", func(t *testing.T) { | |
t.Parallel() | |
ints := []int{1, 2, 3, 4, 5} | |
_ = forEach(ints, func(val *int) error { | |
*val += 1 | |
return nil | |
}) | |
require.Equal(t, []int{2, 3, 4, 5, 6}, ints) | |
}) | |
t.Run("mutating inputs is fine", func(t *testing.T) { | |
t.Parallel() | |
ints := []int{1, 2, 3, 4, 5} | |
_ = forEach(ints, func(val *int) error { | |
*val += 1 | |
return nil | |
}) | |
require.Equal(t, []int{2, 3, 4, 5, 6}, ints) | |
}) |
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.
fixed
We should probably make Mapper respect the |
In #104, a "FailFast" option is likely going to be added to iterators that, on error, stops running additional tasks and returns the error that caused the first failure. "FailFast" seems is a pretty standard description for this behavior, and combining two common options into a single option with a more discoverable name seems like a nice thing to do before 1.0.
Is this still on the radar? |
Yeah, I will refine the pr |
Adding error returning iterators