-
Notifications
You must be signed in to change notification settings - Fork 593
Add StrftimeItems::{parse, parse_to_owned} and more documentation
#1184
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
Conversation
d9b4faf to
bd2ae15
Compare
a031fa3 to
51d009e
Compare
jtmoon79
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.
Nice.
9b2325f to
135fd7d
Compare
Codecov ReportAttention:
Additional details and impacted files@@ Coverage Diff @@
## main #1184 +/- ##
==========================================
- Coverage 93.41% 91.87% -1.55%
==========================================
Files 34 38 +4
Lines 16754 17614 +860
==========================================
+ Hits 15651 16182 +531
- Misses 1103 1432 +329 ☔ View full report in Codecov by Sentry. |
src/format/strftime.rs
Outdated
| #[cfg(any(feature = "alloc", feature = "std"))] | ||
| pub fn parse(self) -> Result<Vec<Item<'a>>, ParseError> { | ||
| let result: Vec<_> = self.collect(); | ||
| if result.iter().any(|i| i == &Item::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.
Instead of first collecting and then iterating again to find Error values, we can do both in one iteration by relying on the FromIter implementation for Result. Something like this:
self.into_iter().map(|item| match item == Item::Error {
false => Ok(item),
true => Err(BAD_FORMAT),
}).collect()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.
Cool, thank you!
| /// # Ok::<(), ParseError>(()) | ||
| /// ``` | ||
| #[cfg(any(feature = "alloc", feature = "std"))] | ||
| pub fn parse_to_owned(self) -> Result<Vec<Item<'static>>, ParseError> { |
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.
Here, too: why should this be added to the public API? What use case does it address that is currently underserved?
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.
The Vec returned by StrftimeItems::parse borrows from the input string. While we have items that can take owned strings they were really not easy to use, or even discover.
Somewhat suggested in #852 (comment) (#1183).
a35592c to
f2b0580
Compare
f2b0580 to
31db358
Compare
|
This is a smallish PR that improves the documentation of @djc you already reviewed a lot the past few days, but how to you feel about getting this one over the finish line? Also because you already gave it one round of review. |
src/format/strftime.rs
Outdated
| /// # Errors | ||
| /// | ||
| /// While iterating [`Item::Error`] will be returned if the format string contains an invalid | ||
| /// or unrecognised formatting specifier. |
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.
"unrecognised" -> "unrecognized".
src/format/strftime.rs
Outdated
| /// # Errors | ||
| /// | ||
| /// While iterating [`Item::Error`] will be returned if the format string contains an invalid | ||
| /// or unrecognised formatting specifier. |
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.
Here, too.
31db358 to
ccde6a2
Compare
Adds
StrftimeItems::parseandStrftimeItems::parse_to_owned.parsereturnsVec<Item<'_>>and can return an error if the format string is invalid.With the new formatting API the
Vechopefully gets more use, and then it may become difficult that is contains references to the format string. That is what theOwnedLiteralandOwnedSpacevariants ofItemare for, but we had no easy way to make those.parse_to_ownedreturnsVec<Item<'static>>.And I added some documentation and doctests.