-
-
Notifications
You must be signed in to change notification settings - Fork 1.3k
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
date: fix %Z specifier does not print TZ abbr #5164
base: main
Are you sure you want to change the base?
Conversation
Cool! Having two additional crates is a bit unfortunate, but if that is the best solution it's alright. I think the CI is failing because you didn't push changes to Cargo.lock. |
Yeah! chrono v0.5 supposed to have a fix for this. will revisit this once v0.5 is released. BTW, I have pushed the cargo.lock changes. |
Still CI is failing. |
While you're fixing the tests, it might be nice to add a comment with this info to the code. |
GNU testsuite comparison:
|
Oops! I accidentally merged main branch again 😅 |
src/uu/date/src/date.rs
Outdated
@@ -397,7 +406,7 @@ fn make_format_string(settings: &Settings) -> &str { | |||
Rfc3339Format::Ns => "%F %T.%f%:z", | |||
}, | |||
Format::Custom(ref fmt) => fmt, | |||
Format::Default => "%c", | |||
Format::Default => "%a %b %-d %X %Z %Y", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Layman's question: Will this not override any locales from LC_TIME
we could use? That could be a blocker for #3143
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, but this change doesn't make it any worse than then the current implementation 😄
GNU testsuite comparison:
|
let format_string = &format_string | ||
.replace("%N", "%f") | ||
.replace("%Z", tz_abbreviation); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is not quite correct, because this means that %f
should be escaped. Ultimately, we might need to completely customize this. I suppose it's good enough for now though.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, this line of code was already there. Cargo fmt has put it in new line 😀. I am interested in solving this though in my next PR.
I didn't get why %f need to be escaped. Did you mean, if there is %f already within the input, it should be escaped?
src/uu/date/src/date.rs
Outdated
@@ -397,7 +406,7 @@ fn make_format_string(settings: &Settings) -> &str { | |||
Rfc3339Format::Ns => "%F %T.%f%:z", | |||
}, | |||
Format::Custom(ref fmt) => fmt, | |||
Format::Default => "%c", | |||
Format::Default => "%a %b %-d %X %Z %Y", |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yes, but this change doesn't make it any worse than then the current implementation 😄
GNU testsuite comparison:
|
added space padding for day in default format
fix Cargo.lock
"Mon Mar 27 08:30:00 2023\n\ | ||
Sat Apr 1 12:00:00 2023\n\ | ||
Sat Apr 15 18:30:00 2023\n", | ||
"Mon Mar 27 08:30:00 UTC 2023\n\ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@tertsdiepraam
I have updated test case test_date_from_stdin, to align with the fix.
GNU testsuite comparison:
|
Is this PR good enough to merge? |
GNU testsuite comparison:
|
GNU testsuite comparison:
|
fixes issue #3756 for date util
description:
chrono
crate ignores %Z specifier. only prints timezone offset. added crateschrono-tz
andiana_time_zone
to calculate the timezone abbreviation and replace %Z specifier with the the abbreviation.