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

Add timezone support to clock module (closes #223) #560

Merged
merged 5 commits into from
Jan 23, 2020
Merged

Add timezone support to clock module (closes #223) #560

merged 5 commits into from
Jan 23, 2020

Conversation

mjec
Copy link
Contributor

@mjec mjec commented Jan 21, 2020

Please forgive my C++ newbiness!

Notably I had to do the subprojects thing to get around HowardHinnant/date#398, and I don't love that I had to write my own formatter in clock.cpp, but I think this should work.

Feedback, and push back, very welcome. Let me know how to improve the code and I will!

@mjec mjec mentioned this pull request Jan 21, 2020
meson.build Show resolved Hide resolved
meson.build Show resolved Hide resolved
@layus
Copy link
Contributor

layus commented Jan 21, 2020

Here are the issues I discovered:

  1. date is not trivial to package, at least with nix. It has incomplete cmake support (work is ongoing apparently) and no meson support. mesonbuild wrapper cannot be used as it downloads stuff on the fly. But that is a concern for package maintainers.

  2. My TZ dir is /etc/zoneinfo as specified by my $TZ_DIR var. apparetly date does not look at TZ_DIR. this sould be upstreamed, but in the meantime this can be a breaking change.

  3. The current setup does not follow LC_TIME (fr_BE.UTF-8 here). I do not know if it follows any locale, or just default to a C/en_US locale. This is a breaking change.

  4. With several clocks, it may be useful to use a different locale per clock. But this can be left out for later.

@layus
Copy link
Contributor

layus commented Jan 21, 2020

From what I see here there is a version of date::format that takes the locale as its first argument. We should use that with std::locale("") or find a way to default to the right locale.

@mjec
Copy link
Contributor Author

mjec commented Jan 21, 2020

With several clocks, it may be useful to use a different locale per clock. But this can be left out for later.

I like this idea, but after noodling with it for a bit my C++ just isn't up to the task :(.

My thinking was having a new struct containing a localtime and locale that gets passed to the fmt::format, and then the locale can be instead of std::locale in formatter::format. However I wasn't able to figure out a way to deal with date::zoned_time being a template type (I think that's the phrase for it?).

@layus
Copy link
Contributor

layus commented Jan 23, 2020

@Alexays I think we have reached the full set of desirable functionalities. Could you have a second look to check C++ style ?

zone = date::locate_zone(config_["timezone"].asString());
} else {
zone = date::current_zone();
if (!fixed_time_zone_) {
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why not just use !config_["timezone"].isString() and removing fixed_time_zone_ var?

Copy link
Contributor

@layus layus Jan 23, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

To avoid going through the json parser each time. May not be worth it.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The json is already parsed, but yeah, never mind.

@Alexays
Copy link
Owner

Alexays commented Jan 23, 2020

LGTM Thanks guys ! ❤️

subprojects/date.wrap Outdated Show resolved Hide resolved
@mjec
Copy link
Contributor Author

mjec commented Jan 23, 2020

@Alexays Have updated the patch URL to be github as well, and rewrote history a little bit to use my personal (rather than work) email on the commits.

Let me know if you'd like me to rewrite the history/squash commits at all.

@Alexays
Copy link
Owner

Alexays commented Jan 23, 2020

Thanks guys, great work 👍

@Alexays Alexays merged commit e9b0365 into Alexays:master Jan 23, 2020
@dancju
Copy link

dancju commented Jan 23, 2020

Will wiki page update?

@Alexays
Copy link
Owner

Alexays commented Jan 23, 2020

Man page is updated, but the wiki still need to be updated, @mjec can you give a shot?

@mjec
Copy link
Contributor Author

mjec commented Jan 23, 2020

Just updated the wiki @Alexays

@mjec
Copy link
Contributor Author

mjec commented Jan 24, 2020

Yes @Nerddan, with a config like:

{
    "modules-right": ["clock#nyc", "clock#sydney"],
    "clock#nyc": {
        "timezone": "America/New_York"
    },
    "clock#sydney": {
        "timezone": "Australia/Sydney"
    }
}

@dancju
Copy link

dancju commented Feb 5, 2020

When will this be packed into the next release?

@Alexays
Copy link
Owner

Alexays commented Feb 5, 2020

@Nerddan By the end of the week

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants