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

Solve "large-dates" ambiguity #683

Open
dennisorlando opened this issue Jun 3, 2024 · 13 comments
Open

Solve "large-dates" ambiguity #683

dennisorlando opened this issue Jun 3, 2024 · 13 comments
Labels
A-formatting Area: formatting A-parsing Area: parsing C-feature-request Category: a new feature (not already implemented)

Comments

@dennisorlando
Copy link

Hi,
bson enables the "large-dates" feature, which means that when you add it to your crate, everything that parsed some time::PrimitiveDateTimes using the plain version of time will potentially break, due to some documented ambiguities.
Is it possible to provide a way to solve those ambiguity, so that we don't have to change our program in order to handle different date-time formats?
Maybe a way to provide the number of characters dedicated to the [year] value?

This is the datetime which causes problems when "large-dates" is enabled:
20240602205731Z

@jhpratt jhpratt added C-feature-request Category: a new feature (not already implemented) A-formatting Area: formatting A-parsing Area: parsing labels Jun 18, 2024
@jhpratt
Copy link
Member

jhpratt commented Jun 18, 2024

This is related to #650. After some thought, I should be able to add a modifier whose value would control whether the extended range can be used. This is what I want eventually anyway; the only difference would be that the default has to be backwards-compatible (essentially opt-out instead of opt-in).

@morganava
Copy link

morganava commented Sep 23, 2024

I recently ran into this exact problem with the simple_asn1 crate when attempting to use bson in the same binary.

Upstream issue: acw/simple_asn1#34

This problem is ultimately the root cause of this issue in the arti-client crate (when bson crate is also present): https://gitlab.torproject.org/tpo/core/arti/-/issues/1632

@jhpratt
Copy link
Member

jhpratt commented Sep 24, 2024

@morganava Feel free to create a pull request if you are able. Otherwise you'll be waiting on me to eventually get around to this (and there is no time frame).

@morganava
Copy link

@jhpratt would the acceptable solution be to make an explicit [short-year] (or some other better name) format specifier which forces years into the range -9999 to 9999 even if time/large-dates is enabled?

@jhpratt
Copy link
Member

jhpratt commented Sep 25, 2024

The solution would be what I described in a previous comment. Basically it would be a modifier rather than its own component.

@dennisorlando
Copy link
Author

What do you mean by modifier? This -> https://docs.rs/modifier ?

@jhpratt
Copy link
Member

jhpratt commented Sep 25, 2024

@dennisorlando See the current reference, which contains terminology and all current supported syntax.

https://time-rs.github.io/book/api/format-description.html

@dennisorlando
Copy link
Author

Minimal reproducable example, which panics when "large-dates" is enabled:

use time::{macros::format_description, PrimitiveDateTime};


fn main() {
    let format = format_description!("[year][month][day][hour][minute][second]");
    let datetime = PrimitiveDateTime::parse("20240602205731", format).unwrap();
    println!("{}", datetime);
}

@dennisorlando
Copy link
Author

Anyways: I'm currently working on this. Is this new modifier suitable? I.e. an explicit 4 digit modifier?

#[non_exhaustive]
#[derive(Debug, Clone, Copy, PartialEq, Eq)]
pub enum YearRepr {
    /// The full value of the year.
    Full,
    /// Standard 4 digit format, to be used when `large-dates` feature is enabled
    Four,
    /// Only the last two digits of the year.
    LastTwo,
}

@dennisorlando
Copy link
Author

I'm continuing the discussion here since the nature of the problem appears to have changed due to 19120ac.

The M.R.C. has become:

use time::{macros::format_description, PrimitiveDateTime};

fn main() {
    let format = format_description!("[year][month][day][hour][minute][second]");
    let datetime = PrimitiveDateTime::parse("-20240602205731", format).unwrap();
    println!("{}", datetime);
}

@dennisorlando
Copy link
Author

dennisorlando commented Oct 27, 2024

Anyways: I saw that you mentioned in #650 the idea of removing the large dates feature in favour of supporting it by default via a new modifier. Wouldn't removing such feature be the best design choice?
By introducing repr:century, a new ambiguity is being introduced due to large-dates. Adding a new modifier which fixes this ambiguity feels weird..

What about this course of action:

  • remove large-dates (via previous deprecation to allow projects to adapt to the newer version)
  • add a "maxdigits" modifier which defaults to 4 (when no sign is present) and 6 (when a sign is present; this should allow compatibility with existing extended year ranges) and lastly 2 when representing a century (I mean, who writes centuries If not with only 2 digits?)

The fact that it's a "maximum" means that it will also support years with less digits, which is useful when such year is separated from the other components by whitespaces, slashes or other characters.
For example, this crashes because the year component has only three digits:
"204/12/12" ("[year]/[month]/[day]")
Even tho it's not ambiguous.

(I'm writing from my phone, sorry for the terrible text that I just wrote)

@dennisorlando
Copy link
Author

Anyways: I saw that you mentioned in #650 the idea of removing the large dates feature in favour of supporting it by default via a new modifier. Wouldn't removing such feature be the best design choice? By introducing repr:century, a new ambiguity is being introduced due to large-dates. Adding a new modifier which fixes this ambiguity feels weird..

What about this course of action:

* remove large-dates (via previous deprecation to allow projects to adapt to the newer version)

* add a "maxdigits" modifier which defaults to 4 (when no sign is present) and 6 (when a sign is present; this should allow compatibility with existing extended year ranges) and lastly 2 when representing a century (I mean, who writes centuries If not with only 2 digits?)

The fact that it's a "maximum" means that it will also support years with less digits, which is useful when such year is separated from the other components by whitespaces, slashes or other characters. For example, this crashes because the year component has only three digits: "204/12/12" ("[year]/[month]/[day]") Even tho it's not ambiguous.

(I'm writing from my phone, sorry for the terrible text that I just wrote)

@jhpratt
(should I have already pinged?)

@jhpratt
Copy link
Member

jhpratt commented Nov 20, 2024

Sorry for the delay! Feel free to ping if I don't respond within ~1 week. Even if there's a reason I hadn't responded (as was the case here due to being busy), I can at least state as such.

By introducing repr:century, a new ambiguity is being introduced due to large-dates. Adding a new modifier which fixes this ambiguity feels weird.

It's actually the same ambiguity, just presented in a different way. The proposed range modifier is a way to have the smallest API for opting in to the ambiguity in the future while being able to opt out for the time being.

The fact that it's a "maximum" means that it will also support years with less digits, which is useful when such year is separated from the other components by whitespaces, slashes or other characters.
For example, this crashes because the year component has only three digits:
"204/12/12" ("[year]/[month]/[day]")
Even tho it's not ambiguous.

This is already supported by using padding:none.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-formatting Area: formatting A-parsing Area: parsing C-feature-request Category: a new feature (not already implemented)
Projects
None yet
Development

No branches or pull requests

3 participants