refactor(data): establish common UnfoldError for shared errors#967
refactor(data): establish common UnfoldError for shared errors#967thomasathorne wants to merge 1 commit intomainfrom
UnfoldError for shared errors#967Conversation
thomasathorne
force-pushed
the
thomasa/common-unfold-errors
branch
from
6e282e0 to
fa21d32
Compare
Codecov Report❌ Patch coverage is ❌ Your patch check has failed because the patch coverage (42.85%) is below the target coverage (80.00%). You can increase the patch coverage or adjust the target coverage. Additional details and impacted files@@ Coverage Diff @@
## main #967 +/- ##
==========================================
- Coverage 89.78% 89.68% -0.11%
==========================================
Files 110 110
Lines 22104 22138 +34
Branches 22104 22138 +34
==========================================
+ Hits 19846 19854 +8
- Misses 1859 1889 +30
+ Partials 399 395 -4 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
thomasathorne
requested review from
NSant215,
emturner,
jobjo,
kurtisc,
vapourismo,
victor-dumitrescu and
zcabter
as code owners
|
Benchmark results for revision 520df2c:
Full results
Compare the results above with those for the default branch. |
| OfComponent(Box<dyn std::error::Error>), | ||
|
|
||
| #[error("Source specific error: {0}")] | ||
| OfSource(Box<dyn std::error::Error>), |
There was a problem hiding this comment.
more curious about your opinion here and learning - claude pointed out that these variants will not support Send and Sync, because dyn Error doesn't, and this prevents compatibility with anyhow::Error or std::io::Error. I think this is only relevant in threaded/async contexts, which wouldn't be the case for Unfold uses?
There was a problem hiding this comment.
I think Claude is misleading you here, if you need Send or Sync you can add those traits to the dyn: eg.
OfSource(Box<dyn std::error::Error + Send + Sync>)
and then as long as you make sure the other error types you want to use do indeed implement those traits you are fine.
There was a problem hiding this comment.
sorry wasn't clear - the edit you suggested is the same that it suggested. I was more curious about the effects of it - i.e. do we care about those compatibility requirements now or in the future. I imagine not because we won't need Unfold in async contexts?
There was a problem hiding this comment.
Hmmm, it's an interesting question, I could imagine there might be a thread running in the rollup node that checks out snapshots (which will mean unfolding them from storage) before processing them in some way, for example while playing refutation games. If the unfold returns an error the thread might handle it all locally in which case there's no need for Send, but it's also conceivable we'd want to pass the error to a logging thread to be reported on. 🤔
3 participants
Relates to TZX-106.
What
Switch the associated type
Errorin theUnfoldtrait with a specific error typeUnfoldError(that includes two 'custom' error variants, one for component-specific errors and one for source-specific errors).Why
In working on the
BlobStoreUnfoldI realised that most of the errors will be common ones that apply to almost anyUnfoldimplementation---basically, incorrect tree shapes. We can save a lot of code duplication by putting these in a common type.Manually Testing
Tasks for the Author