Small fixes to the RFC 3339 parsers

[pitdicker]

We have two RFC 3339 parsers in chrono:

• a strict parser in format::parse::parse_rfc3339, used by DateTime<FixedOffset>::parse_from_rfc_3339 and %+.
• a permissive parser in impl str::FromStr for DateTime<FixedOffset>

This branch fixed a couple of issues with both parsers:

• The separator between a date and time can be a space.
  From the standard:

  NOTE: ISO 8601 defines date and time separated by "T". Applications using this syntax may choose, for the sake of readability, to specify a full-date and full-time separated by (say) a space character.

  The permissive parser already accepted a space, now the strict parser also does.
  The vague wording suggest more characters than 'T' and ' ' might be allowed, but I have stopped at adding just a space.

• Allow 't' as a seperator between date and time.
  From the standard:

  NOTE: Per [ABNF] and ISO8601, the "T" and "Z" characters in this syntax may alternatively be lower case "t" or "z" respectively.

  The strict parser accepted 't', now the relaxed parser also does.

• Don't support UTC as a timezone name in the strict parser.
  RFC 3339 only allows 'Z' or an offset, not UTC.
  It seems support round tripping display <-> datetime #378 accidentally made the strict parser accept this.
  For the relaxed parser it makes sense to accept UTC, as it allows roundtripping with the Debug format of DateTime<Utc>.

• The %+ formatting specifier, which uses the RFC3339 formatting item, has as documentation:

  Same as %Y-%m-%dT%H:%M:%S%.f%:z, i.e. 0, 3, 6 or 9 fractional digits for seconds and colons in the time zone offset.

  This format also supports having a Z or UTC in place of %:z. They are equivalent to +00:00.

  Note that all T, Z, and UTC are parsed case-insensitively.

  The typical strftime implementations have different (and locale-dependent) formats for this specifier. While Chrono's format for %+ is far more stable, it is best to avoid this specifier if you want to control the exact output.

  Al the formatting specifiers noted in this documentation are forgiving with padding. The permissive parser uses the exact formatting items listed here. So I factored out the permissive parser from the FromString implementation, and switched the RFC3339 item to use that parser instead of the strict one.

The remaining differences of the permissive parser with the strict one are:

• Values don't require padding to two digits.
• Years outside the range 0...=9999 are accepted, but they must include a sign.
• UTC is accepted as a valid timezone name/offset.
• There can be spaces between any of the components.
• The colon in the offset may be missing.

[jtmoon79]

I recommend against trim_left as it allows infinite length variety of whitespaces. Very odd and unpleasantly surprising parse successes may occur. I suggest constraining this to zero or one whitespace character.

[pitdicker]

I agree, and this matches your work in #807. But I would like to just match the existing implementation, and adjust whitespace handling an another issue.

[djc]

Instead of this, I think we should merge both parsers, parametrizing them for the behaviors in which their strictness varies.

[pitdicker]

Instead of this, I think we should merge both parsers, parametrizing them for the behaviors in which their strictness varies.

I would agree in theory, but not in this one case. We would have to conditionally allow space padding between all values and literals, and conditionally require zero padding on the values. We can't reuse the standard formatting items as they are too permissive. I will turn out pretty complex and ugly.

The strict parser is pretty simple and somewhat performance-sensitive (used for deserialization). Do we want to introduce 15~20 extra branches into that?

I would keep them separated as they are now.

[codecov]

Codecov Report

Merging #1145 (012c3c8) into 0.4.x (2d2f847) will increase coverage by 0.02%.
The diff coverage is 98.14%.

@@            Coverage Diff             @@
##            0.4.x    #1145      +/-   ##
==========================================
+ Coverage   86.08%   86.10%   +0.02%     
==========================================
  Files          37       37              
  Lines       13501    13522      +21     
==========================================
+ Hits        11622    11643      +21     
  Misses       1879     1879              

Files Changed	Coverage Δ
src/format/mod.rs	75.36% <ø> (ø)
src/format/parse.rs	97.22% <96.15%> (-0.11%)	⬇️
src/datetime/mod.rs	75.70% <100.00%> (+0.09%)	⬆️
src/datetime/tests.rs	96.53% <100.00%> (+0.03%)	⬆️
src/format/scan.rs	99.25% <100.00%> (+0.71%)	⬆️

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

[pitdicker]

@djc Can you please give this PR another round of review? I would like to make some progress on #1083 (offset parsing), and it depends on the cleanups in this one.

[djc]

Would you mind splicing the explanation from the PR description into the messages for the relevant commits? Ideally each commit would explain why the change is made (if it isn't self-evident).

Also, please move the parse_rfc3339_relaxed() definition below the FromStr impl that uses it.

[pitdicker]

Done.

[pitdicker]

The upload to codecov.io failed.

[pitdicker]

Appearently codecov.io can get rate-limited by github if we don't use an upload token. See codecov/codecov-action#557 (comment) and https://community.codecov.com/t/upload-issues-unable-to-locate-build-via-github-actions-api/3954.

@djc Is this something we would want to fix? And I think only you can get the token and set a secret?

[djc]

Again, it's a bit unclear to me why we want to make this change. Maybe add some comments? What about mixed-case UtC or Utc?

[pitdicker]

I am not sure which change you mean. Moving this code from scan::timezone_offset to parse_rfc3339_relaxed?

It seems like a hack to me that if we set the allow_zulu flag to parse Z we also accept the time zone abbreviation UTC. That is what the TimezoneName item is for.

I'll add some comments.

[djc]

I guess the change in general? You're moving support for the UTC offset suffix from timezone_offset() to parse_rfc3339_relaxed(). As far as I can tell, this makes our parsing more strict, so some parses that used to work will now fail. Why do you want to make this change?

[pitdicker]

As far as I can tell, this makes our parsing more strict, so some parses that used to work will now fail.

I have to admit I forgot about it (I wrote the first version end of April). It would only change how the following two items parse:

    /// Offset from the local time to UTC (`+09:00` or `-04:00` or `Z`).
    ///
    /// In the parser, the colon can be omitted and/or surrounded with any amount of whitespace,
    /// and `Z` can be either in upper case or in lower case.
    /// The offset is limited from `-24:00` to `+24:00`,
    /// which is the same as [`FixedOffset`](../offset/struct.FixedOffset.html)'s range.
    TimezoneOffsetColonZ,
    /// Same as [`TimezoneOffsetColonZ`](#variant.TimezoneOffsetColonZ) but prints no colon.
    /// Parsing allows an optional colon.
    TimezoneOffsetZ,

Both are not currently exposed as a formatting specifier, except as part of %+. And they don't mention anything about accepting "UTC". So I thought the impact would be basically none.

Parsing "UTC" was added in #378 for FromStr, with no mention that it would make TimezoneOffsetColonZ and TimezoneOffsetZ accept more inputs.

But good point, that is important!

[pitdicker]

What about mixed-case UtC or Utc?

Good catch, I did not think about this. I have something against case-insensitive compares that accept things like UtC. But it was accepted before. What do you think?

[djc]

Okay, if those are the only entry points I think this risk is acceptable.

Given the limited complexity from accepting mixed-case UTC variations, I'd rather keep it. Maybe add a test to avoid regressing it in the future?

[pitdicker]

Done.

[djc]

Alright, would you mind updating the release notes with items for this and #1229? Then we can push this thing out.