Skip to content

Refactor finisher#476

Merged
NGTmeaty merged 1 commit intointernetarchive:mainfrom
vbanos:finisher-refactor
Sep 11, 2025
Merged

Refactor finisher#476
NGTmeaty merged 1 commit intointernetarchive:mainfrom
vbanos:finisher-refactor

Conversation

@vbanos
Copy link
Copy Markdown
Collaborator

@vbanos vbanos commented Sep 10, 2025

Factor the seed processing out of finisher worker.

Replace all panic() with proper error returning.

The logic remains the same.

Relevant to: #473

Factor the seed processing out of finisher `worker`.

Replace all `panic()` with proper error returning.

The logic remains the same.

Relevant to: internetarchive#473
@codecov-commenter
Copy link
Copy Markdown

Codecov Report

❌ Patch coverage is 50.00000% with 17 lines in your changes missing coverage. Please review.
✅ Project coverage is 56.41%. Comparing base (032cbd6) to head (9fc141d).

Files with missing lines Patch % Lines
internal/pkg/finisher/finisher.go 50.00% 11 Missing and 6 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main     #476      +/-   ##
==========================================
- Coverage   56.45%   56.41%   -0.05%     
==========================================
  Files         130      130              
  Lines        8052     8060       +8     
==========================================
+ Hits         4546     4547       +1     
- Misses       3145     3151       +6     
- Partials      361      362       +1     
Flag Coverage Δ
e2etests 40.63% <50.00%> (-0.03%) ⬇️
unittests 29.44% <0.00%> (-0.03%) ⬇️

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@CorentinB CorentinB added the enhancement New feature or request label Sep 10, 2025
Copy link
Copy Markdown
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR refactors the finisher component by extracting seed processing logic from the worker method into a separate handleSeed function and replacing all panic() calls with proper error handling. The refactoring improves code organization and error handling without changing the underlying business logic.

Key changes:

  • Extracted seed processing logic into a new handleSeed method for better separation of concerns
  • Replaced all panic() calls with proper error returns and structured error messages
  • Enhanced error messages with contextual information including worker ID and seed ID

continue
}
if err := seed.CheckConsistency(); err != nil {
return fmt.Errorf("seed consistency check failed with err: for %s: %s", err.Error(), seed.GetShortID())
Copy link

Copilot AI Sep 10, 2025

Choose a reason for hiding this comment

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

The error message format is awkward with 'with err: for %s'. Consider rephrasing to 'seed consistency check failed for %s: %w' and use error wrapping instead of .Error().

Suggested change
return fmt.Errorf("seed consistency check failed with err: for %s: %s", err.Error(), seed.GetShortID())
return fmt.Errorf("seed consistency check failed for %s: %w", seed.GetShortID(), err)

Copilot uses AI. Check for mistakes.
Copy link
Copy Markdown
Collaborator

@NGTmeaty NGTmeaty left a comment

Choose a reason for hiding this comment

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

Thanks!!

@NGTmeaty NGTmeaty merged commit 8d61429 into internetarchive:main Sep 11, 2025
2 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

enhancement New feature or request

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants