Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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
Draft: [EXPERIMENT] Sketching out a RusticIssueCollector #318
Draft: [EXPERIMENT] Sketching out a RusticIssueCollector #318
Changes from 3 commits
8e227da
0b74f3c
ff5ba3c
8d610bd
d2dfd46
b74d2bf
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Why not using something like
error: RusticError
andseverity: RusticSeverity
with the latter being an enum? I can imagine we can have the exactly identical error which is at some place classified as warning (or even info) and at another place just an error.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know, I would not classify an error at one place as a warning and at the other as an info. An error is an error, and should be clearly communicated as one. If we change the general meaning of an error, then it shouldn't be an error in the first place.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But I do know what you mean, something like: 'File not found'. At one place, it can be irrecoverable, e.g. a config file not found for a backend. At some other place, it maybe can be continued with default settings.
But in that case we should handle that error in-place and not return an error I feel.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Also, important to say here:
RusticIssue
,RusticWarning
, andRusticIssueCollector
should never go over the library boundary. So it's for internal use only, it should not be part of our public API, and we should actually not returnRusticIssue
from any public function - maybe even in general - every function. It should only be used for internal error handling, so we as library authors can judge situations better internally, and give our users errors only in cases that are worthwhile.Hence, I reduced the visibility of all the types in this file to
pub(crate)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We might not have that within this Enum though? 🤔 as that should always be a hard error I think? Not sure though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually I do think that we can have Errors on a deeper level which are shown as warnings on a higher level. Something like backup failed for creating the first snapshot, but we want to continue creating the second one...
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
In this case, we should handle that at the point we get that
error
and start the backup for the secondpath
for example. The failed backup for the first will be output to the user at thematch
of the result (e.g. in a loop) and we start the next one, but give user feedback.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
So I think this is the same topic as above, we shouldn't give
errors
different meanings, I don't know if they are really contextual in that sense. There can be hard and soft errors, for sure. But I think we are going to replace the term soft error here, with anissue
which can result in awarning
orinfo
. But then we need to reserve the termerror
for things that are irrecoverable.Check warning on line 34 in crates/core/src/error/collector.rs
crates/core/src/error/collector.rs#L34
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't like having
String
here. IMO we should just haveRusticError
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same as above, this here is a
warning
,RusticError
should be something that we return to the user, in case we can't recover from something. This is pub here, but I'm not sure if we ever should make RusticWarning, RusticInfo part of our public API. Users ofrustic_core
, alsorustic-rs
should never have to deal with that. They will only geterrors
from us. I think 🤔 If it is recoverable, then we should recover from it, not the users.Check warning on line 68 in crates/core/src/error/collector.rs
crates/core/src/error/collector.rs#L68
Check warning on line 86 in crates/core/src/error/collector.rs
crates/core/src/error/collector.rs#L86
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We might want to adapt it, so we can easier use it from multiple threads, and other contexts, e.g. via Mutexes or Channels.
Check warning on line 120 in crates/core/src/error/collector.rs
crates/core/src/error/collector.rs#L119-L120
Check warning on line 126 in crates/core/src/error/collector.rs
crates/core/src/error/collector.rs#L126
Check warning on line 130 in crates/core/src/error/collector.rs
crates/core/src/error/collector.rs#L130
Check warning on line 138 in crates/core/src/error/collector.rs
crates/core/src/error/collector.rs#L138
Check warning on line 142 in crates/core/src/error/collector.rs
crates/core/src/error/collector.rs#L142
Check warning on line 150 in crates/core/src/error/collector.rs
crates/core/src/error/collector.rs#L150
Check warning on line 154 in crates/core/src/error/collector.rs
crates/core/src/error/collector.rs#L154
Check warning on line 160 in crates/core/src/error/collector.rs
crates/core/src/error/collector.rs#L160
Check warning on line 164 in crates/core/src/error/collector.rs
crates/core/src/error/collector.rs#L164
Check warning on line 168 in crates/core/src/error/collector.rs
crates/core/src/error/collector.rs#L168
Check warning on line 173 in crates/core/src/error/collector.rs
crates/core/src/error/collector.rs#L172-L173
Check warning on line 176 in crates/core/src/error/collector.rs
crates/core/src/error/collector.rs#L176
Check warning on line 180 in crates/core/src/error/collector.rs
crates/core/src/error/collector.rs#L180