refactor(errors)!: improve RusticError display and usage by simonsan · Pull Request #321 · rustic-rs/rustic_core

simonsan · 2024-10-07T21:06:15Z

TODO:

fix todo!("Error transition")
check and group imports (ErrorKind, RusticError, RusticResult)
fix tests
fix clippy
fix build for *nix and osx
check and adapt documentation of errors in functions
remove variants from local errors that are now unused
build rustic-rs against this branch and check, that everything works
check ErrorKind::Command variant and usage, probably not needed variant, as most of it is also Internal -> replace and remove variant

codecov · 2024-10-07T21:18:00Z

Codecov Report

Attention: Patch coverage is 28.12500% with 46 lines in your changes missing coverage. Please review.

Project coverage is 46.2%. Comparing base (6e989b0) to head (163458b).
Report is 1 commits behind head on main.

Files with missing lines	Patch %	Lines
crates/core/src/commands/check.rs	25.0%	30 Missing ⚠️
crates/core/src/repository.rs	33.3%	16 Missing ⚠️

Additional details and impacted files

Files with missing lines	Coverage Δ
crates/core/src/error.rs	`57.1% <ø> (ø)`
crates/core/src/repository.rs	`47.5% <33.3%> (-1.1%)`	⬇️
crates/core/src/commands/check.rs	`63.8% <25.0%> (-2.5%)`	⬇️

... and 15 files with indirect coverage changes

aawsome · 2024-10-12T05:59:59Z

I think this is mixing two things which should be handled completely independent from each other: Channels and error propagation.

Actually, we don't need a channel if we want to collect any kind of information (be it an array of errors, a bool isErr or whatever else) from a called function/method. For this the return value (or maybe a &mut argument) suffices, is simpler and more performant.
We only need a channel to communicate (i.e. send information and this can be an array of errors, a bool or any other result) between threads, i.e. if we spawn threads or use already-spawned threads to do some work.

To be more precise, in this example we could change fn xy(...,err_send: Sender<CheckErrorKind>) -> Result<T> into fn xy(..., is_err: &AtomicBool) -> Result<T> and directly modify is_err or even use fn xy(...) -> Result<(T,bool)> and propagate is_err. This gives exactly the same functionality without using a channel.

IMO there is just one fundamental decision to make: Do we want to return and propagate an error flag or a list of errors?

aawsome · 2024-10-12T06:46:20Z

And the same applies to warning, where we also have to decide:

do we want to just log them and not give the information to the caller?
do we want to give a is_warn bool information to the caller?
do we want to give a list of warnings to the caller?

simonsan · 2024-10-14T17:41:11Z

I think this is mixing two things which should be handled completely independent from each other: Channels and error propagation.

Actually, we don't need a channel if we want to collect any kind of information (be it an array of errors, a bool isErr or whatever else) from a called function/method. For this the return value (or maybe a &mut argument) suffices, is simpler and more performant. We only need a channel to communicate (i.e. send information and this can be an array of errors, a bool or any other result) between threads, i.e. if we spawn threads or use already-spawned threads to do some work.

To be more precise, in this example we could change fn xy(...,err_send: Sender<CheckErrorKind>) -> Result<T> into fn xy(..., is_err: &AtomicBool) -> Result<T> and directly modify is_err or even use fn xy(...) -> Result<(T,bool)> and propagate is_err. This gives exactly the same functionality without using a channel.

IMO there is just one fundamental decision to make: Do we want to return and propagate an error flag or a list of errors?

I think collecting all errors and returning them from the check_repository function makes sense, as we are processing a collection of data with multiple ways to fail. Repository::check() itself though, should only return a single error stating that the check failed imho.

I agree, I used the channel here, because I got confused by the parallel iterator. But instead of a for_each, we can just use map and collect the results. Then we can e.g. partition the results and return a RusticResult<Result<() | Vec<RusticWarning>, Vec<RusticError>>>. Where the inner Result is for soft-errors/warnings (that would result in a warning or just a logged error, depending on its severity that we would determine in the callers match). And the outer result a hard-error.

The RusticWarning here should stay internal use only, and never propagate to the user of the lib, it's only for logging purposes and further handling - I would even think, we don't even need it and could log a warning directly where it is important. So we can easily find the spot where the warning was emitted. The Vec<RusticError> should also not become part of the Repository API as the users should not need to handle multiple errors and decide on the severity on their own - for them the RusticResult<()> with a single error should be the contract. So they can rely on either the check being ok, or the check having failed.

That being said, #224 (comment) my opinion is still varying. The tendency goes to the nested result type though, I think that can be a solution.

In general, I think, that we want to build some kind of generator that produces Result<T, E> values that we can evaluate in the caller. I think that was where the thought of a channel came from for the Errors. But I imagine it more to be a thread running check and putting the results into a channel to be evaluated by the main thread.

Signed-off-by: simonsan <14062932+simonsan@users.noreply.github.com>

crates/core/tests/snapshots/errors__error_display.snap

Signed-off-by: simonsan <14062932+simonsan@users.noreply.github.com>

crates/core/src/commands/cat.rs

crates/core/src/commands/check.rs

See https://github.com/rosetta-rs/string-rosetta-rs Signed-off-by: simonsan <14062932+simonsan@users.noreply.github.com>

Signed-off-by: simonsan <14062932+simonsan@users.noreply.github.com>

## 🤖 New release * `rustic_backend`: 0.4.2 -> 0.5.0 (⚠️ API breaking changes) * `rustic_core`: 0.5.5 -> 0.6.0 (⚠️ API breaking changes) * `rustic_testing`: 0.2.3 -> 0.3.0 (✓ API compatible changes) ### ⚠️ `rustic_backend` breaking changes ``` --- failure enum_missing: pub enum removed or renamed --- Description: A publicly-visible enum cannot be imported by its prior path. A `pub use` may have been removed, or the enum itself may have been renamed or removed entirely. ref: https://doc.rust-lang.org/cargo/reference/semver.html#item-remove impl: https://github.com/obi1kenobi/cargo-semver-checks/tree/v0.36.0/src/lints/enum_missing.ron Failed in: enum rustic_backend::error::LocalBackendErrorKind, previously in file /tmp/.tmp0sSY8G/rustic_backend/src/error.rs:90 enum rustic_backend::error::RestErrorKind, previously in file /tmp/.tmp0sSY8G/rustic_backend/src/error.rs:67 enum rustic_backend::error::BackendAccessErrorKind, previously in file /tmp/.tmp0sSY8G/rustic_backend/src/error.rs:10 enum rustic_backend::error::RcloneErrorKind, previously in file /tmp/.tmp0sSY8G/rustic_backend/src/error.rs:43 --- failure module_missing: pub module removed or renamed --- Description: A publicly-visible module cannot be imported by its prior path. A `pub use` may have been removed, or the module may have been renamed, removed, or made non-public. ref: https://doc.rust-lang.org/cargo/reference/semver.html#item-remove impl: https://github.com/obi1kenobi/cargo-semver-checks/tree/v0.36.0/src/lints/module_missing.ron Failed in: mod rustic_backend::error, previously in file /tmp/.tmp0sSY8G/rustic_backend/src/error.rs:1 ``` ### ⚠️ `rustic_core` breaking changes ``` --- failure inherent_method_missing: pub method removed or renamed --- Description: A publicly-visible method or associated fn is no longer available under its prior name. It may have been renamed or removed entirely. ref: https://doc.rust-lang.org/cargo/reference/semver.html#item-remove impl: https://github.com/obi1kenobi/cargo-semver-checks/tree/v0.36.0/src/lints/inherent_method_missing.ron Failed in: LocalDestination::remove_dir, previously in file /tmp/.tmp0sSY8G/rustic_core/src/backend/local_destination.rs:129 LocalDestination::remove_file, previously in file /tmp/.tmp0sSY8G/rustic_core/src/backend/local_destination.rs:152 LocalDestination::create_dir, previously in file /tmp/.tmp0sSY8G/rustic_core/src/backend/local_destination.rs:171 LocalDestination::set_times, previously in file /tmp/.tmp0sSY8G/rustic_core/src/backend/local_destination.rs:189 LocalDestination::set_user_group, previously in file /tmp/.tmp0sSY8G/rustic_core/src/backend/local_destination.rs:237 LocalDestination::set_uid_gid, previously in file /tmp/.tmp0sSY8G/rustic_core/src/backend/local_destination.rs:283 LocalDestination::set_permission, previously in file /tmp/.tmp0sSY8G/rustic_core/src/backend/local_destination.rs:324 LocalDestination::set_extended_attributes, previously in file /tmp/.tmp0sSY8G/rustic_core/src/backend/local_destination.rs:385 LocalDestination::set_length, previously in file /tmp/.tmp0sSY8G/rustic_core/src/backend/local_destination.rs:466 LocalDestination::create_special, previously in file /tmp/.tmp0sSY8G/rustic_core/src/backend/local_destination.rs:521 LocalDestination::read_at, previously in file /tmp/.tmp0sSY8G/rustic_core/src/backend/local_destination.rs:599 LocalDestination::write_at, previously in file /tmp/.tmp0sSY8G/rustic_core/src/backend/local_destination.rs:663 RusticError::into_inner, previously in file /tmp/.tmp0sSY8G/rustic_core/src/error.rs:46 RusticError::backend_error, previously in file /tmp/.tmp0sSY8G/rustic_core/src/error.rs:61 ``` <details><summary>Changelog</summary> ## `rustic_backend` <blockquote> ## [0.5.0](rustic_backend-v0.4.2...rustic_backend-v0.5.0) - 2024-11-18 ### Added - *(async)* add `async_compatible` methods to identify backend compatibility ([#355](#355)) - add 'yandex-disk' to enabled opendal services and update opendal to 0.50.2 ([#360](#360)) ### Other - *(error)* enhance error logging and output formatting ([#361](#361)) - *(backend)* simplify code in local backend ([#362](#362)) - *(backend)* migrate from `backoff` to `backon` ([#356](#356)) - *(error)* improve error messages and file handling ([#334](#334)) - *(deps)* lock file maintenance rust dependencies ([#345](#345)) - *(deps)* [**breaking**] upgrade to new conflate version ([#300](#300)) - *(errors)* [**breaking**] Improve error handling, display and clean up codebase ([#321](#321)) </blockquote> ## `rustic_core` <blockquote> ## [0.6.0](rustic_core-v0.5.5...rustic_core-v0.6.0) - 2024-11-18 ### Added - *(async)* add `async_compatible` methods to identify backend compatibility ([#355](#355)) ### Fixed - prevent overwriting hot repository on init ([#353](#353)) ### Other - *(error)* enhance error logging and output formatting ([#361](#361)) - *(deps)* remove Derivative and replace with Default impl due to RUSTSEC-2024-0388 ([#359](#359)) - *(error)* improve error messages and file handling ([#334](#334)) - *(deps)* lock file maintenance rust dependencies ([#345](#345)) - *(deps)* remove cdc and switch to rustic_cdc ([#348](#348)) - *(deps)* [**breaking**] upgrade to new conflate version ([#300](#300)) - *(errors)* [**breaking**] Improve error handling, display and clean up codebase ([#321](#321)) </blockquote> ## `rustic_testing` <blockquote> ## [0.3.0](rustic_testing-v0.2.3...rustic_testing-v0.3.0) - 2024-11-18 ### Added - *(async)* add `async_compatible` methods to identify backend compatibility ([#355](#355)) ### Other - *(errors)* [**breaking**] Improve error handling, display and clean up codebase ([#321](#321)) </blockquote> </details> --- This PR was generated with [release-plz](https://github.com/MarcoIeni/release-plz/). Co-authored-by: rustic-release-plz[bot] <182542030+rustic-release-plz[bot]@users.noreply.github.com>

simonsan added A-errors Area: error handling needs improvement C-enhancement Category: New feature or request A-commands Area: Related to commands in `rustic_core` labels Oct 7, 2024

simonsan requested a review from aawsome October 7, 2024 21:06

simonsan changed the title ~~[DRAFT] refactor(commands): improve error handling in check command~~ [DRAFT] refactor(commands/errors): improve error handling in check command Oct 8, 2024

simonsan changed the title ~~[DRAFT] refactor(commands/errors): improve error handling in check command~~ [DRAFT] refactor(errors): improve error handling in rustic_core Oct 16, 2024

simonsan force-pushed the refactor/check-error-handling branch from 4871b5c to 6ee6ef8 Compare October 23, 2024 16:45

simonsan added 5 commits October 23, 2024 19:02

Removing anyhow

101f898

move error enums to their local files

7cdd50e

Error handling

ac5a261

fix typo

d8421db

style: format changelog

b690a80

simonsan force-pushed the refactor/check-error-handling branch from 6ee6ef8 to b690a80 Compare October 23, 2024 17:34

simonsan added 11 commits October 24, 2024 01:55

RusticErrors for Choose, Local and Utils

b5a365e

Signed-off-by: simonsan <14062932+simonsan@users.noreply.github.com>

RusticErrors for Local

36c0ca1

Signed-off-by: simonsan <14062932+simonsan@users.noreply.github.com>

RusticErrors for Local

93f99b2

Signed-off-by: simonsan <14062932+simonsan@users.noreply.github.com>

style: cargo fmt

96b4516

Signed-off-by: simonsan <14062932+simonsan@users.noreply.github.com>

remove leftovers from check command error handling draft

d51a6ca

Signed-off-by: simonsan <14062932+simonsan@users.noreply.github.com>

RusticErrors for Check

8af508d

Signed-off-by: simonsan <14062932+simonsan@users.noreply.github.com>

RusticErrors for Cat

0ecf288

Signed-off-by: simonsan <14062932+simonsan@users.noreply.github.com>

add context for cat

17f8a35

Signed-off-by: simonsan <14062932+simonsan@users.noreply.github.com>

RusticErrors for Dump incl Display for NodeType

b507c5b

Signed-off-by: simonsan <14062932+simonsan@users.noreply.github.com>

RusticErrors for forget

612ccd1

Signed-off-by: simonsan <14062932+simonsan@users.noreply.github.com>

RusticErrors for repair

cfaf6de

Signed-off-by: simonsan <14062932+simonsan@users.noreply.github.com>

simonsan mentioned this pull request Oct 24, 2024

refactor(error): improve error messages and file handling #334

Merged

RusticErrors for crypto, need std feature for impl Error for aead

9d1534b

Signed-off-by: simonsan <14062932+simonsan@users.noreply.github.com>

simonsan added 2 commits October 30, 2024 16:04

Make it possible to use multiple different issue urls for an error

5d41598

Signed-off-by: simonsan <14062932+simonsan@users.noreply.github.com>

update error kind and error display impl

8e8abe1

Signed-off-by: simonsan <14062932+simonsan@users.noreply.github.com>

simonsan added this to the NEXT milestone Oct 30, 2024

simonsan added 4 commits October 30, 2024 20:07

replace placeholders based on context keys in guidance

8dab388

Signed-off-by: simonsan <14062932+simonsan@users.noreply.github.com>

Use the context placeholders in the guidance messages

72f1782

Signed-off-by: simonsan <14062932+simonsan@users.noreply.github.com>

attach more context to errors that are already RusticErrors

b98e35b

Signed-off-by: simonsan <14062932+simonsan@users.noreply.github.com>

add instructions about forwarding RusticError in a map_err

17180a6

Signed-off-by: simonsan <14062932+simonsan@users.noreply.github.com>

simonsan commented Oct 31, 2024

View reviewed changes

crates/core/tests/snapshots/errors__error_display.snap Outdated Show resolved Hide resolved

simonsan commented Oct 31, 2024

View reviewed changes

crates/core/tests/snapshots/errors__error_display.snap Outdated Show resolved Hide resolved

update error display

125e7e2

Signed-off-by: simonsan <14062932+simonsan@users.noreply.github.com>

simonsan commented Nov 5, 2024

View reviewed changes

crates/core/src/commands/cat.rs Outdated Show resolved Hide resolved

simonsan commented Nov 5, 2024

View reviewed changes

crates/core/src/commands/check.rs Outdated Show resolved Hide resolved

simonsan commented Nov 5, 2024

View reviewed changes

crates/core/src/commands/check.rs Outdated Show resolved Hide resolved

simonsan and others added 10 commits November 5, 2024 17:50

refactor: use ecow instead of smol_str due to recommendation

8baaf40

See https://github.com/rosetta-rs/string-rosetta-rs Signed-off-by: simonsan <14062932+simonsan@users.noreply.github.com>

Remove Command variant from ErrorKind.

04feb7a

Signed-off-by: simonsan <14062932+simonsan@users.noreply.github.com>

fix(clippy): lints

ed4c005

Signed-off-by: simonsan <14062932+simonsan@users.noreply.github.com>

upgrade thiserror to 2.0.0

7a910ab

Signed-off-by: simonsan <14062932+simonsan@users.noreply.github.com>

Merge branch 'main' into refactor/check-error-handling

37dd5cb

Merge branch 'main' into refactor/check-error-handling

abb5973

cache: use warning and ignore non-existing cache dirs

c330148

shorten error message

2aa5b50

fix some issues after review

388c9ca

Signed-off-by: simonsan <14062932+simonsan@users.noreply.github.com>

clippy: allow many lines for impl Display

74a0bd3

Signed-off-by: simonsan <14062932+simonsan@users.noreply.github.com>

simonsan removed the request for review from aawsome November 16, 2024 23:00

simonsan removed the S-waiting-for-review Status: PRs waiting for review label Nov 16, 2024

simonsan merged commit 40ad287 into main Nov 16, 2024

simonsan deleted the refactor/check-error-handling branch November 16, 2024 23:28

rustic-release-plz bot mentioned this pull request Nov 16, 2024

chore: release #354

Merged

aawsome mentioned this pull request Nov 17, 2024

fix: prevent overwriting hot repository on init #353

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

refactor(errors)!: improve RusticError display and usage#321

refactor(errors)!: improve RusticError display and usage#321
simonsan merged 136 commits intomainfrom
refactor/check-error-handling

simonsan commented Oct 7, 2024 •

edited

Loading

Uh oh!

codecov bot commented Oct 7, 2024 •

edited

Loading

Uh oh!

aawsome commented Oct 12, 2024

Uh oh!

aawsome commented Oct 12, 2024

Uh oh!

simonsan commented Oct 14, 2024 •

edited

Loading

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

Conversation

simonsan commented Oct 7, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

codecov bot commented Oct 7, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Codecov Report

Uh oh!

aawsome commented Oct 12, 2024

Uh oh!

aawsome commented Oct 12, 2024

Uh oh!

simonsan commented Oct 14, 2024 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

simonsan commented Oct 7, 2024 •

edited

Loading

codecov bot commented Oct 7, 2024 •

edited

Loading

simonsan commented Oct 14, 2024 •

edited

Loading