Fix large-dates ambiguity by creating a repr:four modifier

[dennisorlando]

This is linked to #683

This PR creates a new [year] modifier, i.e. repr:four, which represents a full year which is forced to be 4 digits long. This is different from the default repr:full, which has a variable amount of digits depending if the large-dates feature is enabled, thus causing parsing ambiguities that might produce errors in crates who depend oh time.rs.

Tests have still to be written for this PR, thus it's not ready to be merged, but I opened this pull request anyways so that someone else might write them.
Namings and comments might also need to be discussed.

(I'll make a rebase of the commits when this PR is ready to be merged)

[dennisorlando]

...what's up with the failing checks?

[codecov]

Codecov Report

All modified and coverable lines are covered by tests ✅

Project coverage is 97.7%. Comparing base (4a74924) to head (4d99768).
Report is 51 commits behind head on main.

❗ Current head 4d99768 differs from pull request most recent head 19120ac

Please upload reports for the commit 19120ac to get more accurate results.

Additional details and impacted files

@@           Coverage Diff           @@
##            main    #709     +/-   ##
=======================================
- Coverage   97.8%   97.7%   -0.1%     
=======================================
  Files         81      83      +2     
  Lines       9378    8980    -398     
=======================================
- Hits        9169    8773    -396     
+ Misses       209     207      -2     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

[morganava]

For reviewability, you may want to do the cargo fmt changes first in their own separate commit, avoid polluting the Created tests by... commit with non-test changes.

[morganava]

And there should probably be an accompanying merge request on https://github.com/time-rs/book to clarify how to resolve the large-dates ambiguity, update the [year] component diagaram, etc in https://time-rs.github.io/book/api/format-description.html#components

[dennisorlando]

Not sure about the formatting check; it appears that the cargo fmt in the Github Action is only complaining about a different order of some imports. I tried running cargo fmt --all on my repo but sadly it doesn't appear to fix it; I guess there's a version mismatch between my cargo fmt version and the one used in Github Actions

[morganava]

@dennisorlando it looks like the CI is using rust nightly (at least for that step):

  fmt:
    name: Formatting
    runs-on: ubuntu-latest
    if: (github.event_name == 'pull_request' && github.event.pull_request.head.repo.fork) || github.event_name == 'push'

    steps:
      - name: Checkout sources
        uses: actions/checkout@v4

      - name: Install toolchain
        uses: dtolnay/rust-toolchain@nightly
        with:
          components: rustfmt

      - name: Check formatting
        run: cargo fmt --all -- --check
        env:
          RUSTFLAGS: --cfg bench

[dennisorlando]

@dennisorlando it looks like the CI is using rust nightly (at least for that step)

Thanks! I rerun cargo fmt with an updated nightly installation and it worked.
Should a "cargo fmt --version" be added to the formatting workflow or is it unnecessary? e.g.

      - name: Install toolchain
        <...>

      - name: Print version
        run: cargo fmt --version

      - name: Check formatting
        run: cargo fmt --all -- --check
        env:
          RUSTFLAGS: --cfg bench

[jhpratt]

Overall, nice work! I have a few nits. More generally, I would prefer this be named repr:narrow with the corresponding YearRepr::Narrow variant. I feel this better captures the intent when combined with the large-dates feature.

Finally, can you remove the formatting commit and rebase instead? It was something I hadn't kept up to date with on nightly, hence the issue.

[jhpratt]

Given that formatting can fail, it would be a good idea to add a test for that situation.

[dennisorlando]

Is there a specific section I can put such test in? I can't figure out where to put it

[jhpratt]

You can create a new method if one doesn't seem obvious. I want to say RFC 3339 is the only case where formatting can fail currently, so it's quite likely this is the case.

[dennisorlando]

Overall, nice work! I have a few nits. More generally, I would prefer this be named repr:narrow with the corresponding YearRepr::Narrow variant. I feel this better captures the intent when combined with the large-dates feature.

Finally, can you remove the formatting commit and rebase instead? It was something I hadn't kept up to date with on nightly, hence the issue.

On it;
I just wanted to double your thoughts about the "Narrow" naming as I feel like it might be unintuitive, especially when googling it with a query like "time.rs force year to be four digits long".
Perhaps something like "Standard" would be a better fit? When large-dates is disabled, people reading Narrow might associate it to something which is narrower then the standard 4 digit format, and thus skipping to read it's documentation

[jhpratt]

"standard" works for me.

[jhpratt]

After some more recent work, namely adding repr:century, I think it would be best if this were actually its own modifier. If that were the case then it could apply to either repr:century or repr:full.

I don't believe it's documented how to add a new modifier, so let me know if you need any guidance. If you prefer, I can also tackle it myself.

[dennisorlando]

After some more recent work, namely adding repr:century, I think it would be best if this were actually its own modifier. If that were the case then it could apply to either repr:century or repr:full.

I don't believe it's documented how to add a new modifier, so let me know if you need any guidance. If you prefer, I can also tackle it myself.

Hmm I have a hard time decoupling it from repr: It is, after all, a representation of the year.
How do you think it should be called? Something along the lines of [year force-4-digits:true] ?

Maybe a digit modifier? It would create a lot of conflicts with the existing code tho

[jhpratt]

With the addition or repr:century, I'm viewing it as a way to control the range of supported values rather than a dedicated repr of its own volition. Otherwise we'd need to have a repr:full_standard and repr:century_standard, likewise for any further unforeseen variants.

My thought was range:extended and range:standard, with the former being default for backwards compatibility. That would be changed in the next breaking release, bringing behavior in line with most people's expectations.

[dennisorlando]

With the addition or repr:century, I'm viewing it as a way to control the range of supported values rather than a dedicated repr of its own volition. Otherwise we'd need to have a repr:full_standard and repr:century_standard, likewise for any further unforeseen variants.

My thought was range:extended and range:standard, with the former being default for backwards compatibility. That would be changed in the next breaking release, bringing behavior in line with most people's expectations.

how should range:standard affect the different representations (e.g. when used with repr:century or repr:last_two)?
(P.S: I should be able to work on this this weekend)

[jhpratt]

For repr:century, it should output two digits if and only if the year is in 0..=9_999, failing otherwise. It would have no effect on repr:last_two.

P.S: I should be able to work on this this weekend

Awesome! 🙂