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

feat: Improve summary handling and error reporting in repository checks #369

Draft
wants to merge 7 commits into
base: feat/summary
Choose a base branch
from

Conversation

simonsan
Copy link
Contributor

@simonsan simonsan commented Nov 25, 2024

Enhance the check_repository and check_cache_files functions to return a Summary, improve concurrency handling, and strengthen error logging for better reporting. Refactor summary handling in related functions for consistency.

Based on #365

@simonsan simonsan force-pushed the feat/summary-check branch 2 times, most recently from 42ae627 to 688f349 Compare November 25, 2024 13:32
Copy link

codecov bot commented Nov 25, 2024

Codecov Report

Attention: Patch coverage is 33.05785% with 81 lines in your changes missing coverage. Please review.

Project coverage is 42.0%. Comparing base (4b3d073) to head (a2aee81).
Report is 2 commits behind head on feat/summary.

Files with missing lines Patch % Lines
crates/core/src/commands/check.rs 30.4% 73 Missing ⚠️
crates/core/src/commands/merge.rs 0.0% 5 Missing ⚠️
crates/core/src/archiver.rs 75.0% 2 Missing ⚠️
crates/core/src/repository.rs 66.6% 1 Missing ⚠️
Additional details and impacted files
Files with missing lines Coverage Δ
crates/core/tests/integration.rs 84.0% <ø> (+4.0%) ⬆️
crates/core/src/repository.rs 42.4% <66.6%> (+0.1%) ⬆️
crates/core/src/archiver.rs 58.9% <75.0%> (-0.8%) ⬇️
crates/core/src/commands/merge.rs 0.0% <0.0%> (ø)
crates/core/src/commands/check.rs 57.4% <30.4%> (-4.8%) ⬇️

... and 5 files with indirect coverage changes

---- 🚨 Try these New Features:

@simonsan
Copy link
Contributor Author

I'm still debating with myself, if passing in a summary: Arc<Mutex<Summary>> into the check functions would be better, instead of returning the Summary and initializing it at the beginning of the functions, calling complete at the end. For now, we have clearer contexts and timings due to that. But we could also add that functionality based on passing in a summary. We could impl start/stop on Summary, for timing purposes.

I do think it's a bit cleaner separation, to return a Summary and then merge it with the Summary up in the call stack.

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.

1 participant