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

Make NaiveTime::from_str support HH:MM as well as HH:MM:SS #619

Closed
wants to merge 1 commit into from

Conversation

jamespharaoh
Copy link

Times are commonly written without seconds, NaiveTime::from_str should recognise this format as well. This adds that functionality.

This makes parsing data structures which include times easier, since you would otherwise need to specify a format string or construct the NaiveTime yourself.

@esheppa
Copy link
Collaborator

esheppa commented Jul 3, 2022

Thanks for this @jamespharaoh - apologies for the long delay. I like where you are going with this as it would be interesting to make the various FromStr implementations more flexible, however there are some tradeoffs which I'd like to seek comment on before going ahead with this:

a) Parse/Display Roundtrip - if we allow multiple valid input formats, then it is impossible guarantee a identity when combining parse and display. (This may also be the case currently, which we could fix instead if desired)
b) How we view the FromStr and Display impls compared to the format and parse_from_str methods. For now the FromStr and Display impls have some sensible defaults, but it could be interesting to have FromStr try a couple of different common patterns in order (hopefully in an unambiguous way) to make parsing common date/time formats easier (especially for new users of the library) at the cost of some performance.

This PR falls within the latter option and if it is decided to go down that path, it might be interesting to have an array of arrays of Item<'static> and then try each one in order - this would also avoid adding the Truncated variant to Item.

@esheppa esheppa added this to the 0.5 milestone Jul 3, 2022
@djc
Copy link
Contributor

djc commented Jul 4, 2022

a) Parse/Display Roundtrip - if we allow multiple valid input formats, then it is impossible guarantee a identity when combining parse and display. (This may also be the case currently, which we could fix instead if desired)

At least for this case, I think parsing HH:MM as if it was written as HH::MM::00 makes sense. The identity round trip through Display doesn't seem very important to me at least in this case where there is a sensible default we can fill in.

@esheppa esheppa removed the design label Jul 4, 2022
@esheppa
Copy link
Collaborator

esheppa commented Jul 4, 2022

Thanks for the feedback @djc. In this case, if possible @jamespharaoh - would you mind implementing this using an array or arrays of Item<'static>? My line of thinking here is opening the way to allow for example 1500 to parse as a time as well in future. (Or alternatively, if we can make a case of Item::Truncated being generally useful, then maybe keep it as is)

@pitdicker
Copy link
Collaborator

Closing in favor of #1181.

@pitdicker pitdicker closed this Jul 20, 2023
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

4 participants