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

API changes: avoid relying on global state? #8

Open
c-cube opened this issue Sep 22, 2018 · 9 comments
Open

API changes: avoid relying on global state? #8

c-cube opened this issue Sep 22, 2018 · 9 comments
Milestone

Comments

@c-cube
Copy link
Member

c-cube commented Sep 22, 2018

there are some global mutations in the lib (e.g. Time_zone.change) which make reasoning about the library complicated.

I think it'd be nice to instead have a ?tz:Time_zone.t argument in functions that make use of the global state, to overload it.

@pmetzger
Copy link
Member

This seems reasonable, though could we get rid of global state entirely?

@c-cube
Copy link
Member Author

c-cube commented Sep 22, 2018

I think there's room for discussion there:

  • one aspect is compatibility (how much breakage is good for users? breaking a bit might be fine, but if all code has to be rewritten it's not very good)
  • global state along with Time_zone.on kind of emulates dynamic scoping, which may be convenient when doing a lot of operation in a given timezone (rather than specifying the parameter every time).

The optional parameter is a smoother transition path, but it's true that we need to know where we want to go eventually.

@pmetzger
Copy link
Member

I'm not sure it's the best idea, but can one create a functor parameterized over a datum like a timezone (rather than another module) so you get a set of functions specialized to a particular timezone? (You can certainly do a poor man's version of this with lexical closures.) Forgive me for asking what might be a stupid question, although I've been using OCaml for a year or so every once in a while a question like this comes up in my head from lack of knowledge.

@loxs
Copy link
Collaborator

loxs commented Sep 22, 2018

I personally like the idea of a functor. It means that not all code will have to be rewritten but only when opening the module, which will now be equivalent to setting the time zone in the current implementation. To me this is a reasonable change for a major version bump.

@loxs loxs added this to the 3.0 milestone Sep 22, 2018
@Drup
Copy link
Member

Drup commented Sep 22, 2018

My vote goes to trying to remove the global state has much as possible, even if it makes breaking the API. It's really a distasteful mechanics that generates bugs far too easily. In general, dates should all be in UTC anyway. only the printing should be in another timezone.

@pmetzger
Copy link
Member

It is usually best if things in date libraries are maintained internally in UTC or TAI, yes.

I'm not sure that a functor of the type I'm mentioning above is even possible, I've never seen it done. The equivalent is easily done with lexical closures, but this isn't something I've observed in other OCaml code. It was more of a question about whether it's a thing that can be done or not.

@loxs
Copy link
Collaborator

loxs commented Sep 22, 2018

Yes, probably what @Drup suggests is the best way forward.

@c-cube
Copy link
Member Author

c-cube commented Sep 22, 2018

Could the dates embed the timezone in which they were defined? I'm really no expert but it conveys the intent better and may be more robust in some cases.

For example if you live in Europe, set an alarm every morning at 7, and go to a conference somewhere in the US, storing the alarm event in UTC would be wrong?

Of course, comparing dates, or other operations, could convert them to UTC first. I think this kind of design decision would be helped by looking at what other languages do.

@loxs
Copy link
Collaborator

loxs commented Sep 22, 2018

This example with the alarm is another type of time data type, which is “without timezone” and just depends on the context. It’s not in UTC, it’s in no time zone at all. And yes, I think we need to support this (if not already supported)

On the other hand, when the time is “zone aware”, storing it in UTC always is what I have been doing and have read on the topic, and generally never had issues with this way. Of course, I am also not an expert (yet) so yes, I’ll have a look at various languages and databases (where I have mostly dealt with date and time)

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

No branches or pull requests

4 participants