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

The default time format is changed after localization date support #828

Closed
Tracked by #750
zwpaper opened this issue Mar 28, 2023 · 9 comments
Closed
Tracked by #750

The default time format is changed after localization date support #828

zwpaper opened this issue Mar 28, 2023 · 9 comments
Labels
good first issue Good for newcomers help wanted Extra attention is needed

Comments

@zwpaper
Copy link
Member

zwpaper commented Mar 28, 2023

after merging #820, the default time format is changed,
maybe we should switch back to the previous format, and add a flag for the localization date.

image

cc @meain

@meain
Copy link
Member

meain commented Mar 31, 2023

Yup, we should stick to original format. Changing the defaults is a bad idea. Maybe add another option to --date flag, maybe something like locale?

@zwpaper zwpaper added help wanted Extra attention is needed good first issue Good for newcomers labels Apr 2, 2023
@zwpaper
Copy link
Member Author

zwpaper commented Apr 15, 2023

@scarf005 do you have some cycle to look into this?

@scarf005
Copy link
Contributor

scarf005 commented Apr 15, 2023

is the upper section of the screenshot before #820 and the lower section of the screenshot after #820?
in the PR i've changed val.format("%c").to_string() to val.format_localized("%c", locale).to_string(), this could've been the cause of the regression. I'm not sure why the same format works differently, however.

@zwpaper
Copy link
Member Author

zwpaper commented Apr 15, 2023

@scarf005 it may be the difference between localed time format or not.

So if we could add the localed format under a option, that would be great.

@scarf005
Copy link
Contributor

is DateFlag::Date for default format and DateFlag::Formatted for user defined formatting?
if so, maybe we could replace "%c" with "%a %b %d %T %Y" to simulate previous behavior.

scarf@TG02:~$ LANG=c date +"%a %b %d %T %Y"
Sat Apr 15 22:09:51 2023

@zwpaper
Copy link
Member Author

zwpaper commented Apr 16, 2023

scarf@TG02:~$ LANG=c date +"%a %b %d %T %Y"
Sat Apr 15 22:09:51 2023

this seems to be a good solution for this, but like the --time-style=locale(https://man7.org/linux/man-pages/man1/ls.1.html) in GNU ls, maybe we should also better provide a locale value to date option, and opt-in locale instead of locale by default.

@scarf005
Copy link
Contributor

https://github.com/coreutils/coreutils/blob/cc95246ee2ed674cd407f0f80fd77f3364012458/src/ls.c#L2397-L2404
looks like GNU coreutils default --time-style to locale.

I do agree that providing --time-style is a good idea, but a pointer on how to make implement it would be very helpful. for example

  • how to add --time-style flag
  • how should it handle formatting style from config file (code-wise)

@zwpaper
Copy link
Member Author

zwpaper commented Apr 16, 2023

oh, I have not noticed that the locale is the default! then we could align to it.

how to add --time-style flag

we would now stick to the date flag, and migrate to time-style later, that would be not much change needed.

how should it handle formatting style from config file (code-wise)

how about:

        if let Date::Date(val) = self {
            match &flags.date {
				// leave this to not localized
                DateFlag::Date => val.format_localized("%c", locale).to_string(),
				// add a Locale and format with "%a %b %d %T %Y" to simulate the previous format with locale support
                DateFlag::Relative => HumanTime::from(*val - Local::now()).to_string(),
                DateFlag::Iso => {
                    // 365.2425 * 24 * 60 * 60 = 31556952 seconds per year
                    // 15778476 seconds are 6 months
                    if *val > Local::now() - Duration::seconds(15_778_476) {
                        val.format("%m-%d %R").to_string()
                    } else {
                        val.format("%F").to_string()
                    }
                }
                DateFlag::Formatted(format) => val.format_localized(format, locale).to_string(),

@zwpaper
Copy link
Member Author

zwpaper commented Apr 30, 2023

close as fixed by #840

@zwpaper zwpaper closed this as completed Apr 30, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
good first issue Good for newcomers help wanted Extra attention is needed
Projects
None yet
Development

No branches or pull requests

3 participants