Make time parsing more lenient

[CaptainMaso]

I was trying to parse some values for time that I had previously been using chrono for, and found that the human readable parsing for time values required the subsecond field to be present. I've added tests for this in the serde section, and then fixed them. All the tests still pass, but I guess this is probably a slightly controversial change as previously denied values are now accepted.

Please note that this change affects the Time, PrimitiveDateTime and OffsetDateTime parsing and deserialising, but it is not intended to affect the serialising/formatting.

The alternative is to require people to write their custom format_description, but this results in slightly annoying code to handle internal values when it's a pretty simple addition.

time::serde::format_description!(user_times, Time, "[hour]:[minute]:[second][optional [. [subsecond]]");

struct MyTime(time::Time);

impl<'de> serde_with::DeserializeAs<'de,time::Time> for MyTime {
    fn deserialize_as<D>(deserializer: D) -> std::result::Result<time::Time, D::Error>
    where
        D: serde::Deserializer<'de> {
        user_times::deserialize(deserializer)
    }
}

#[serde_as]
#[derive(Debug, Clone, serde::Deserialize)]
pub struct Config {
    #[serde_as(as = "Vec<MyTime>")]
    pub transmit_times_of_day: Vec<time::Time>,
}

[codecov[bot]]

Codecov Report

Merging #580 (8d2f8d7) into main (e83d01f) will decrease coverage by 0.2%. The diff coverage is 87.6%.

:exclamation: Current head 8d2f8d7 differs from pull request most recent head 5e625ea. Consider uploading reports for the commit 5e625ea to get more accurate results

@@           Coverage Diff           @@
##            main    #580     +/-   ##
=======================================
- Coverage   95.7%   95.5%   -0.2%     
=======================================
  Files         79      79             
  Lines       8795    8999    +204     
=======================================
+ Hits        8421    8594    +173     
- Misses       374     405     +31     

Impacted Files	Coverage Δ
time/src/serde/mod.rs	96.5% <ø> (ø)
time/src/formatting/formattable.rs	91.2% <78.6%> (-8.8%)	:arrow_down:
time/src/formatting/mod.rs	99.2% <80.0%> (-0.8%)	:arrow_down:
time/src/format_description/component.rs	93.3% <92.9%> (-6.7%)	:arrow_down:
time/src/date.rs	100.0% <100.0%> (ø)
time/src/date_time.rs	94.6% <100.0%> (+<0.1%)	:arrow_up:
time/src/format_description/modifier.rs	100.0% <100.0%> (ø)
time/src/time.rs	100.0% <100.0%> (ø)
time/src/utc_offset.rs	100.0% <100.0%> (ø)

:mega: We’re building smart automated test selection to slash your CI/CD build times. Learn more

[jhpratt]

Can you add tests for the compact representation as well? Other than that, this PR looks good.

[CaptainMaso]

It's probably worth a second review at this point. I've also added skipping formatting of optional values for some types of components under certain conditions.

The skippable components are seconds (when seconds past minute is 0), subseconds (when subseconds is 0), offset_hour (when the offset is UTC), offset_minute (when offset minutes past hour is 0), and offset_second (when offset seconds past minute is 0), and literals are always skippable. All other components are never skippable, as there isn't really a conceptual idea of when an hour item should be skipped, but it's pretty clear that when seconds or subseconds is 0, they can probably be skipped during formatting.

The logic is as follows:

• Always format items unless they are wrapped in an Optional component.
• Once wrapped in an Optional component, check if all sub-components are skippable (e.g. compound components require all internal components to be skippable to skip the whole component).
• If all sub-components are skippable, skip the formatting and emit 0 bytes written.
• Otherwise, format as usual.

This means that literals that are wrapped in Optional are always skipped in formatting, so maybe we should include a modifier that makes it always formatted, but skippable in parsing? This use case is probably rare, as optional literals are usually paired with an actual parsing item that should control whether it is skipped or not.

[jhpratt]

I've also added skipping formatting of optional values for some types of components under certain conditions.

That's a significant change in behavior that is contrary to what documentation states. Optional items are always present when formatting; they are only optional when parsing. I'd like to keep this PR to what was originally included, as making parsing more lenient and changing formatting behavior are two very different things.

maybe we should include a modifier that makes it always formatted, but skippable in parsing

This is what [optional] is.

[CaptainMaso]

That's a fair assessment. I don't think that the work is inherently incorrect, but if the docs explicitly say the opposite then I guess that there's no real way to change that until 0.4 rolls around. I mostly did it because some tests assumed a round-trip conversion should be possible. I'll split the formatting work off into a separate PR and let that hang around until such a time that it might be usable. Until then I'll convert the tests I wrote to explicitly fail if the formatting changes, and get this one in a state to be merged.

[jhpratt]

Closing as inactive.