-
Notifications
You must be signed in to change notification settings - Fork 1.9k
Fix DateTimeWrapper parsing
#6952
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
Parse default `chrono::DateTime::to_string` format
...to ensure `DateTimeWrapper::from_str` can successfully parse `DateTimeWrapper::to_string` output
| ParsedExpression::Datetime(DatetimeExpression::Constant(dt_str.parse().map_err( | ||
| |err: chrono::ParseError| { | ||
| CollectionError::bad_input(format!( | ||
| "failed to parse date-time {dt_str:?}: {err}" | ||
| )) | ||
| }, | ||
| )?)) |
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.
This change is optional, but would have been nice to have better errors during parsing. Would have trivialized solving this issue even more.
| // Attempt to parse default to-string format | ||
| .or_else(|_| chrono::DateTime::from_str(s)) |
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.
According to chrono documentation, FromStr implementation:
Accepts a relaxed form of RFC3339. A space or a ‘T’ are accepted as the separator between the date and time parts. Additional spaces are allowed between each component.
Which seems exactly what we want here? Maybe we can even put this as the first parser we try, before even parse_from_rfc3339.
DateTimeWrappercould not parse it's ownto_stringoutput. Oops. Fixed now.All Submissions:
devbranch. Did you create your branch fromdev?New Feature Submissions:
cargo +nightly fmt --allcommand prior to submission?cargo clippy --all --all-featurescommand?Changes to Core Features: