-
Notifications
You must be signed in to change notification settings - Fork 88
Pattern error types #180
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
base: master
Are you sure you want to change the base?
Pattern error types #180
Conversation
1ef8c5d to
0974796
Compare
tgross35
left a comment
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.
Added thiserror crate for the ease of work with the error messages, if this would be a problem please let me know. Had to set version, due to rustc 1.63 used, because latest versions require rustc 1.68.
It's not a problem per se, but I think we may as well avoid the extra dependencies here. All we need is the Display impl, which is only a few extra lines to write by hand.
Also while coding wondered if maybe merging the newly created enum with the PatternError wouldn't be cleaner, but not sure. Showcase below
pub enum PatternError { /// Wildcard should be only `*` or `**` #[error("Patter error at position {pos}: wildcards are either regular `*` or recursive `**` ")] InvalidWildcards {pos: usize}, ... }
This crossed my mind as well. Since pos is common to all error kinds, I think the current structure is okay. But I would remove the Eq and Copy traits so we have more flexibility adding fields to the enum in the future.
Could you also make PatternError non_exhaustive?
6449305 to
7db8619
Compare
tgross35
left a comment
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 think this looks pretty good with the two small nits here. Unfortunately we can't merge this right away though; this is of course a breaking change, so we need to wait on some decisions about how to handle 1.0 first.
Cargo.toml
Outdated
| tempfile = "3" | ||
| doc-comment = "0.3" | ||
| doc-comment = "0.3" |
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.
Missing trailing newline (changes to this file can just be dropped)
src/lib.rs
Outdated
| PatternErrorKind::InvalidWildcards => { | ||
| "Wildcards are either regular `*` or recursive `**`" | ||
| } | ||
| PatternErrorKind::InvalidRecursiveWildcards => { | ||
| "Recursive wildcards must form a single path component" | ||
| } | ||
| PatternErrorKind::InvalidRange => "Invalid range pattern", |
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.
Error strings usually start with a lowercase letter
7db8619 to
50ad0f1
Compare
|
Sure thing, thanks for the feedback. I will try to work on some other things that could bring the glob closer to 1.0 :) |
41b0ddb to
50ad0f1
Compare
50ad0f1 to
759ed3a
Compare
Fixes #166
Initial proposal of the restructure of the PatterError, inspired by code proposed in Issue #166.
Added thiserror crate for the ease of work with the error messages, if this would be a problem please let me know.
Had to set version, due to rustc 1.63 used, because latest versions require rustc 1.68.
Also while coding wondered if maybe merging the newly created enum with the PatternError wouldn't be cleaner, but not sure. Showcase below