Skip to content
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

Add StrftimeItems::{parse, parse_to_owned} and more documentation #1184

Merged
merged 4 commits into from Feb 10, 2024

Conversation

pitdicker
Copy link
Collaborator

Adds StrftimeItems::parse and StrftimeItems::parse_to_owned.

parse returns Vec<Item<'_>> and can return an error if the format string is invalid.

With the new formatting API the Vec hopefully gets more use, and then it may become difficult that is contains references to the format string. That is what the OwnedLiteral and OwnedSpace variants of Item are for, but we had no easy way to make those.
parse_to_owned returns Vec<Item<'static>>.

And I added some documentation and doctests.

@pitdicker pitdicker marked this pull request as ready for review July 11, 2023 16:21
@pitdicker pitdicker force-pushed the strftime_parse branch 3 times, most recently from a031fa3 to 51d009e Compare July 15, 2023 06:31
Copy link
Contributor

@jtmoon79 jtmoon79 left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nice.

@pitdicker pitdicker force-pushed the strftime_parse branch 2 times, most recently from 9b2325f to 135fd7d Compare September 21, 2023 20:10
@codecov
Copy link

codecov bot commented Sep 21, 2023

Codecov Report

Attention: 19 lines in your changes are missing coverage. Please review.

Comparison is base (58f1f37) 93.41% compared to head (31db358) 91.87%.
Report is 56 commits behind head on main.

❗ Current head 31db358 differs from pull request most recent head ccde6a2. Consider uploading reports for the commit ccde6a2 to get more accurate results

Files Patch % Lines
src/naive/datetime/mod.rs 66.66% 6 Missing ⚠️
src/datetime/mod.rs 86.95% 3 Missing ⚠️
src/format/mod.rs 70.00% 3 Missing ⚠️
src/format/strftime.rs 97.67% 2 Missing ⚠️
src/month.rs 0.00% 2 Missing ⚠️
src/duration.rs 99.69% 1 Missing ⚠️
src/offset/utc.rs 0.00% 1 Missing ⚠️
src/weekday.rs 0.00% 1 Missing ⚠️
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.
📢 Have feedback on the report? Share it here.

src/format/strftime.rs Show resolved Hide resolved
#[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) {
Copy link
Contributor

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()

Copy link
Collaborator Author

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> {
Copy link
Contributor

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?

Copy link
Collaborator Author

@pitdicker pitdicker Sep 22, 2023

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).

@pitdicker
Copy link
Collaborator Author

This is a smallish PR that improves the documentation of StrftimeItems and adds two methods to make it easier to re-use its result: parse and parse_to_owned.

@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.

/// # Errors
///
/// While iterating [`Item::Error`] will be returned if the format string contains an invalid
/// or unrecognised formatting specifier.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"unrecognised" -> "unrecognized".

/// # Errors
///
/// While iterating [`Item::Error`] will be returned if the format string contains an invalid
/// or unrecognised formatting specifier.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Here, too.

@pitdicker pitdicker merged commit 58a2149 into chronotope:main Feb 10, 2024
29 of 35 checks passed
@pitdicker pitdicker deleted the strftime_parse branch February 10, 2024 19:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants