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

Purpose of Locale? #61

Closed
chexxor opened this Issue Jul 14, 2017 · 13 comments

Comments

Projects
None yet
2 participants
@chexxor
Copy link
Contributor

chexxor commented Jul 14, 2017

I can't find the explanation of Locale. Am I missing it? Based on my interpretation from reading code, it seems to be fundamentally flawed. Is there plans to move this towards the ThreeTen model of "Locale"?

Current explanation of Locale:

-- | The name of a date/time locale. For example: "GMT", "MDT", "CET", etc.
newtype LocaleName = LocaleName String
data Locale = Locale (Maybe LocaleName) Minutes
data LocalValue a = LocalValue Locale a
type LocalDateTime = LocalValue DateTime

My problem with this is that timezone names (e.g. "GMT", "MDT", "CET") are a poor way of assigning timzone-offsets to a DateTime value. If the DateTime in England is in Summer, then "BST" offset applies, but if it's in Winter, then "GMT" offset applies. So, it makes more sense to join a DateTime with a representative-city than with a timezone. Actually rendering the DateTime with the offset considered, then, requires a map from representative-city to time-offset for that DateTime's specific value. (This map can be quite large, as seen in MomentJS's timezone file.)

The ThreeTen library follows this concept of using representative-city to specify a DateTime's offset, but it calls it "ZoneId". The ZonedDateTime explains a bit further.

@garyb

This comment has been minimized.

Copy link
Member

garyb commented Jul 14, 2017

I don't personally have any plans to do anything with it; for the purpose I designed it for it works exactly as intended - working with date/time values that are in a locale seems like a fundamentally bad idea to me, so I convert to UTC, and then covert back if necessary afterwards, for presentation purposes. The inner value is supposed to be a value in UTC, the offset is supposed to be how much to alter that by when printing a localised date/time.

That doesn't mean it's good though, it's primitive by design. 😄

I'm not sure I understand your point about GMT / BST though. They are different, yes, but you can "look" at a date in BST at any time of year, regardless of the time zone being observed. Knowing that some DateTime was in BST allows you to print that information when rendering a timestamp - that's the purpose of the string really, and is also why it is optional, since sometimes you don't care about that, you just need to know how far off UTC a provided date was.

The library has no opinion about where you get your locale information from, so there's no reason you can't have something like a representative city -> locale function defined, as far as I can see? Likewise you can provide locale conversions easily enough.

@garyb

This comment has been minimized.

Copy link
Member

garyb commented Jul 14, 2017

I don't personally have any plans to do anything with it

That doesn't mean I'm not open to changing it by the way, just saying that it's dumb by design currently.

@chexxor

This comment has been minimized.

Copy link
Contributor

chexxor commented Jul 14, 2017

My main concern with the current LocalDateTime is that it depends on implementations of functions to keep that offset up-to-date. For example, if we do math on current LocalDateTime, like adding days, the timezone-offset would need to be recalculated on every application of addDays, because the new DateTime would exist in a different timezone-offset due to daylight-savings.

Alternatively, if the timezone is associated with a representative-city, the offset doesn't need to be recalculated on every operation, and only needs to be calculated when "rendering" the LocalDateTime.

We could make Locale required and use Locale to mean the representative-city for timezone.

newtype LocaleName = LocaleName String -- "America/Chicago"
-- or --
newtype LocaleName = AmericaChicago | EuropeParis | ... | ...
data Locale = Locale LocaleName
data LocalValue a = LocalValue Locale a
type LocalDateTime = LocalValue DateTime

-- Apply the timezone offset
-- Might need second argument - the representative-city to offset map/data structure
unLocalDateTime :: LocalDateTime -> DateTime
@chexxor

This comment has been minimized.

Copy link
Contributor

chexxor commented Jul 15, 2017

I'm curious what you think of a solution like the one I proposed the previous comment, as it seems to be philosophically quite different than the current design. Your design says an offset should be part of the LocalDateTime and my proposed design says the location of the DateTime should be part of the LocalDateTime.

This isn't a high priority for me right now, so I wouldn't send a PR soon, but it's been an interest of mine for awhile. Also, it would be nice to know where this design would fit in this library - as a redesign of the current LocalDateTime or as a new module.

@garyb

This comment has been minimized.

Copy link
Member

garyb commented Jul 15, 2017

It sounds like a nice idea, but I think it would be a lot of work to maintain. It seems like you'd need a lookup table for daylight savings changes for every location supported, for each year, and would have to keep updating that over time too?

@chexxor

This comment has been minimized.

Copy link
Contributor

chexxor commented Jul 15, 2017

I believe ThreeTen would need to maintain such a data structure, yeah.
We could reuse an existing data structure. Moment-timezone has timezone-definition data structures we could use. They have algorithms for greatly compressing/decompressing it - which we could rewrite in PS.

@chexxor

This comment has been minimized.

Copy link
Contributor

chexxor commented Jul 15, 2017

Actually, looks like moment-timezone's data structures are a processed form of the data maintained by iana.org/time-zones.

@chexxor

This comment has been minimized.

Copy link
Contributor

chexxor commented Jul 15, 2017

Looks like there are opportunities to minify the timezone rules by filtering the JSON-friendly format of the IANA files to contain only the 4 or 8 of timezones you expect the application's users to use, and to contain only the most current rules (the IANA has 60 years of rules due to to political bodies changing their timezones and offsets).

@garyb

This comment has been minimized.

Copy link
Member

garyb commented Jul 16, 2017

I would suggest maybe we do this in a contrib library, and perhaps just delete this Locale next time we do a round of breaking changes. It was perhaps a mistake to ever include locale at all in this library, since just a Tuple or record would work equally well for the usage I described above. This library is supposed to be about the data structures for date/time, and locale is more of a presentation concern, so it's not really in the same category.

I suggest contrib, as it's not something I'd want to take responsibility for maintaining - sorry. I have way too many libraries that I'm somewhat responsible for already, and those don't even have a messy "let's potentially change the rules every year" component 😉

@chexxor

This comment has been minimized.

Copy link
Contributor

chexxor commented Jul 17, 2017

Sounds good. Do you want to leave this issue open until a separate locale/timezone solution becomes available?

@garyb

This comment has been minimized.

Copy link
Member

garyb commented Jul 17, 2017

Sure thing, it'll be useful for anyone who has similar queries and drops by.

If you have any interest in doing that library I can set up a contrib thing and give you access if you like?

@chexxor

This comment has been minimized.

Copy link
Contributor

chexxor commented Jul 17, 2017

I'm quite busy for a bit, but will update here when I've got something worth sharing.

@garyb

This comment has been minimized.

Copy link
Member

garyb commented May 22, 2018

Locale will be removed in the 0.12 release, so closing this. I hope to revisit some of the many issues this library has properly one day still, but for now I'm just going to remove all the problematic stuff and just keep the basics.

@garyb garyb closed this May 22, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment