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

Support for strftime formatters in 0.3 #341

Closed
SergioBenitez opened this issue Jul 30, 2021 · 8 comments
Closed

Support for strftime formatters in 0.3 #341

SergioBenitez opened this issue Jul 30, 2021 · 8 comments
Labels
A-docs Area: documentation A-format-description Area: format description

Comments

@SergioBenitez
Copy link

SergioBenitez commented Jul 30, 2021

I'm really surprised to see that strftime formatting options are not available in time. While the new formatting format is more readable, it's also significantly more verbose, and rewriting existing strftime formatting strings in this new format is error prone:

- static FORMAT: &str = "%a, %d %b %Y %H:%M:%S GMT";
+ static FORMAT: &[time::format_description::FormatItem<'_>] = format_description!(
+     "[weekday repr:short], [day] [month repr:short] [year base:calendar] \
+     [offset_hour]:[offset_minute]:[offset_second] GMT"
+ );

The documentation is also lacking, so I have no idea if these are actually equivalent. Examples would help, especially if they are exhaustive. For example, month says:

The long format is the full English name of the month, while the short format is the first three letters of it.

For the short format, is it written "Jan" or "jan" or "JAN"?

@jhpratt
Copy link
Member

jhpratt commented Jul 30, 2021

Would a tool like the one for 0.1 to 0.2 alleviate concerns over transitioning?

I don't dispute that it's more verbose, that's almost the point. I personally prefer to have something that can be both easily read and written, not just written (sorta like regex). Given the intermediate representation, I suppose it would be possible to have a parser for the older format. I'm not a huge fan of having two completely different formats, though, so that should probably fall to a third-party crate.

With regard to documentation, you're the one that asked for a release without waiting for full docs 😛 The short format is "Jan" — all it does is truncate it to three letters, still following standard English capitalization.

@jhpratt jhpratt added A-docs Area: documentation A-format-description Area: format description labels Jul 30, 2021
@jhpratt
Copy link
Member

jhpratt commented Aug 1, 2021

I've just updated the aforementioned tool to support converting 0.2 to 0.3. The only compatibility note here is that %C (the century-like part of the year) has been removed, so there's no equivalent. Everything other valid format should be converted to an equivalent format.

The format you included becomes

[weekday repr:short], [day] [month repr:short] [year] [hour]:[minute]:[second] GMT

Note that the output of the tool does not include any modifiers that are default — in this case the year's repr defaults to the calendar year.

@SergioBenitez
Copy link
Author

SergioBenitez commented Aug 8, 2021

year seems to work a bit differently that I would have expected, and presumably differently than it used to work, when the year has only two digits. I would expect that a format of [year] used to parse 20 would parse the year as 20. Instead, parsing fails unless I use repr:last_two. But this seems wrong. The year 20 is a valid year...the year 20.

The documentation here is incredibly hand-wavy:

Note that when parsing, if only the last two digits of the year are present, the value returned may not be what was expected — if the return is successful at all (it's not guaranteed).

Parsing needs to be very well specified. This is a blocker to migrating cookie to time v0.3. Tests are failing unexpectedly, and while I can work around the failing tests, I have no confidence that what I'm writing is correct because of documentation like this. In any case, this feels like a bug to me, though one that I'm not sure the implementation of Parsed allows fixing in an obvious manner. In particular, Parsed separates the year into year and year_last_two; what should 20 fit into in the above example?

@jhpratt
Copy link
Member

jhpratt commented Aug 8, 2021

The padded value has a width of 4. You can choose between padding with zeroes, spaces, or having no padding at all. The default is to pad the value with zeroes.

This is key. 20 is invalid for [year]. If you don't care about padding, you need to specify it with [year padding:none]. I've just confirmed this works:

use time::macros::format_description;
use time::Date;

fn main() {
    let date = Date::parse(
        "19-002",
        &format_description!("[year padding:none]-[ordinal]"),
    )
    .unwrap();
    dbg!(date);
}

The Parsed struct has the year and year_last_two fields, though the latter is currently not considered when constructing a Date. I'm not willing ruling out assuming it to be between (say) 1970-2069, hence the "not guaranteed" statement. Which field the value gets assigned to depends on whether repr is full or last_two.


Formatting and parsing will be better documented. Were it not for your request, I likely wouldn't have even released 0.3 yet, as I wanted to have better documentation first.

@SergioBenitez
Copy link
Author

SergioBenitez commented Aug 8, 2021

This is key. 20 is invalid for [year]. If you don't care about padding, you need to specify it with [year padding:none]. I've just confirmed this works:

This is pretty weird and really unexpected. In any case, the converter should take this into account, then. I don't think %Y has the same restriction, and yet it is converted to [year].

Formatting and parsing will be better documented. Were it not for your request, I likely wouldn't have even released 0.3 yet, as I wanted to have better documentation first.

With respect to formatting, you've deviated substantially from the universally accepted, battle tested, understood norm. Yes, I totally missed this in pushing for the release, and I would have advocated for keeping the previous formatting spec, but even so I don't think this is a mater of "better" documentation but about precision. There's enough documentation to be useful, but not enough to know if my code is correct. This is also the piece of time's documentation that appears to have the most effort put into it and so I would imagine that in considering what to pour effort into prior to a release, it would not have been particularly reconsidered. But who knows. Nevertheless, I think releasing 0.3 was absolutely the right thing to do.

@jhpratt
Copy link
Member

jhpratt commented Aug 8, 2021

This is pretty weird and really unexpected. In any case, the converter should take this into account, then. I don't think %Y has the same restriction, and yet it is converted to [year].

After quickly looking, this was likely a bug in 0.2. This function did not take the length of the consumed padding into account the same way it was here. Had I noticed it early on I probably would have fixed it. At this point it would effectively be a breaking change for 0.2. The converter could take this into account, but then the formatting wouldn't be the same, only parsing would.

With respect to formatting, you've deviated substantially from the universally accepted, battle tested, understood norm.

Unquestionably. I still believe this was the best decision, as it allows for far more flexibility, expansion possibilities, and future performance improvement. Single-letter components was error prone when writing, nearly impossible to read, and had near-zero possibility for expansions like case insensitive parsing — something relatively easy to implement now.

This is also the piece of time's documentation that appears to have the most effort put into it and so I would imagine that in considering what to pour effort into prior to a release, it would not have been particularly reconsidered.

It's hard to say definitively of course, but I likely would have improved documentation in that area, as I know it's still not thorough enough (I just wanted something basic to at least have it documented).

@jhpratt
Copy link
Member

jhpratt commented Oct 18, 2021

Coming back around to this. I do not see this as something I want to support. All components and modifiers are currently public under the format_description module. Because of this, there is nothing special about the format_description::parse function. If there is demand for a strftime equivalent then it could plausibly be implemented in a third-party crate.

@MiSawa
Copy link

MiSawa commented Mar 6, 2022

Shameless plug: I ended up with creating a crate doing it. It isn't "production-ready" quality (because I don't need that quality for my use case) and probably should be considered as a never-ending beta/experimental state, but in case anyone interested: https://github.com/MiSawa/time-fmt/

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-docs Area: documentation A-format-description Area: format description
Projects
None yet
Development

No branches or pull requests

3 participants