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

fix: do not cache formatting errors #450

Merged
merged 1 commit into from
Oct 15, 2024
Merged

fix: do not cache formatting errors #450

merged 1 commit into from
Oct 15, 2024

Conversation

brianmcgee
Copy link
Member

This was a regression introduced when the new walk.Reader approach was applied.

When a formatter errors out during processing, we should not update those files in the cache to ensure they are retried during later invocations.

I've extended the walk.ReleaseFunc to accept an optional error which can be used to indicate the file participated in a batch that had errors when being formatted.

This allows the hook implementation to decide what it should do.

Closes #449

Signed-off-by: Brian McGee [email protected]

Copy link
Collaborator

@jfly jfly left a comment

Choose a reason for hiding this comment

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

Code LGTM. I'm not totally understanding the tests, but I'm giving you a shipit because I trust that things are correct!

cmd/root_test.go Outdated Show resolved Hide resolved
cmd/root_test.go Outdated Show resolved Hide resolved
cmd/root_test.go Outdated Show resolved Hide resolved
walk/cached.go Outdated Show resolved Hide resolved
walk/cached.go Outdated Show resolved Hide resolved
@brianmcgee brianmcgee force-pushed the fix/no-cache-errors branch 2 times, most recently from dbda676 to 5814173 Compare October 14, 2024 14:05
@brianmcgee brianmcgee requested a review from jfly October 14, 2024 14:06
cmd/root_test.go Outdated Show resolved Hide resolved
cmd/root_test.go Show resolved Hide resolved
cmd/root_test.go Show resolved Hide resolved
This was a regression introduced when the new `walk.Reader` approach was applied.

When a formatter errors out during processing, we should not update those files in the cache to ensure they are retried during later invocations.

I've extended the `walk.ReleaseFunc` to accept an optional `error` which can be used to indicate the file participated in a batch that had errors when being formatted.

This allows the hook implementation to decide what it should do.

Closes #449

Signed-off-by: Brian McGee <[email protected]>
@brianmcgee brianmcgee requested a review from jfly October 14, 2024 14:57
@brianmcgee brianmcgee merged commit cf35361 into main Oct 15, 2024
27 checks passed
@brianmcgee brianmcgee deleted the fix/no-cache-errors branch October 15, 2024 06:42
brianmcgee added a commit that referenced this pull request Oct 15, 2024
We try to apply formatters to all files on a best-effort basis, continuing if formatting any particular batch of files fails.

This fix ensures that if any formatting errors occur, the process will exit with an error.

Closes #450

Signed-off-by: Brian McGee <[email protected]>
brianmcgee added a commit that referenced this pull request Oct 15, 2024
We try to apply formatters to all files on a best-effort basis, continuing if formatting any particular batch of files fails.

This fix ensures that if any formatting errors occur, the process will exit with an error.

Closes #450

Signed-off-by: Brian McGee <[email protected]>
brianmcgee added a commit that referenced this pull request Oct 16, 2024
We try to apply formatters to all files on a best-effort basis, continuing if formatting any particular batch of files fails.

This fix ensures that if any formatting errors occur, the process will exit with an error.

Closes #450

Signed-off-by: Brian McGee <[email protected]>
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.

Do not cache files that were part of a batch that had errors when formatting
2 participants