Pp refresh - #3
Conversation
| input, | ||
| regex, | ||
| } | ||
| pub const fn new(input: &'de str, regex: Regex) -> Deserializer { |
There was a problem hiding this comment.
Do you have a good use-case for making this const?
AFAIK creating a Regex isn't possible in a const context anyways.
|
|
||
| /// An error that occurred during deserialization. | ||
| #[derive(Debug)] | ||
| #[non_exhaustive] |
There was a problem hiding this comment.
i don't think we should do this. Marking an error enum as non exhaustive makes some devs more lazy about error handling and there should be a good reason for doing this. The error handling of this crate is already quite weak and in would be better to improve this instead of allowing users to ignore errors.
| #[inline] | ||
| fn fmt(&self, f: &mut Formatter<'_>) -> std::fmt::Result { | ||
| use Error::*; | ||
| match self { |
There was a problem hiding this comment.
This make the code more difficult to read without any benefit.
| pub fn from_str<'a, T>(input: &'a str, regex: &str) -> std::result::Result<T, Error> where T: Deserialize<'a> { | ||
| let regex = Regex::new(®ex).map_err(Error::BadRegex)?; | ||
| from_str_regex(input, regex) | ||
| #[inline] |
There was a problem hiding this comment.
Do you have any benchmarks that show noticeable performance improvements marking this and other functions as inline?
| where | ||
| T: Deserialize<'input>, | ||
| { | ||
| let rex = Regex::new(regex).map_err(Error::BadRegex)?; |
There was a problem hiding this comment.
Name shadowing can be confusing, but i don't think this applies to regex here and replacing it with rex doesn't improve this.
|
|
||
| impl Value { | ||
| fn parse<T>(&self) -> Result<T> where T: FromStr { | ||
| #[allow(clippy::map_err_ignore)] |
There was a problem hiding this comment.
It's better to not ignore this error and make the parser error available in the caller instead of ignoring this problem...
2 participants
Hi,
What do you think about the attached (mostly trivial) commits that address some style issues reported by
cargo fmtandcargo clippywith a couple of non-default Clippy categories allowed? (most of the clippy::restriction ones, some clippy::pedantic ones, and clippy::nursery) Of course, as the author of the de-regex crate the decision as to which of these make sense and which are just busy-work lies entirely with you :)Thanks again, and keep up the great work!
G'luck,
Peter