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

Doesn't compile with time 1.9 #9

Closed
dbaynard opened this issue Feb 24, 2018 · 21 comments
Closed

Doesn't compile with time 1.9 #9

dbaynard opened this issue Feb 24, 2018 · 21 comments

Comments

@dbaynard
Copy link

Hello,

time 1.9 hides the member functions of of ParseTime and FormatTime classes (haskell/time@afe728e) which breaks timezone-series. I've had a cursory glance and can't see a straightforward way for the current design of timezone-series to continue to work. It might be worth raising an upstream issue if you can't, either.

Rather frustrating; I was hoping to use some of the new time functions…

@AshleyYakeley
Copy link

Well this is awkward. I didn't think anyone needed to create their own FormatTime or ParseTime instances.

Why does timezone-series need them?

@dbaynard
Copy link
Author

I'm not 100% — I haven't studied the code in detail — but having grepped a bit in the source it looks like it doesn't need them, but just exports the instances for downstream users. If that's the case it's probably worth just deleting the instances from timezone-series.

@ygale
Copy link
Owner

ygale commented Feb 25, 2018

Yes timezone-series does need to provide those instances.

This library was written with the intention of it being incorporated into the time library, not to remain a separate library. It provides a type that represents a fundamental concept of time that is missing from the time library, and thus it is used by a fairly significant number of time library users.

To ease the ability to be incorporated into the time library, it attempts to conform to the coding conventions throughout the time library, One of those is that every time type provides FormatTIme and ParseTime instances. I believe that people are most likely using those instances, too, though I can't know for sure.

Seems to me that the way forward is one of the following:

  1. If @AshleyYakeley agrees, finally incorporate timezone-series into the time library. Best in my opinion. OR,
  2. The time library exports the required methods in an Internal module, as per standard practice. (In my opinion that should be done even if we do (1), but of course that's also up to @AshleyYakeley.) OR,
  3. Incorporate entire copies of the FormatTime and ParseTime classes into the timezone-series library, with some choice of name overlap or non-overlap that hopes to minimize code breakage and fixability. I'd probably have to incorporate copies of all the instances from the time library, too, so people could fix their broken code by just hiding the import of the original time library classes and using mine instead. Yuck. Besides being an awful mess, this will be a maintenance nightmare going forward. I really hope I don't have to do this.

In any case, I'll publish a transitional intentionally-broken version of timezone-series that partially works with time 1.9 but doesn't support the FormatTime and ParseTime classes. It will have a deprecation notice right from the start, suggesting an immediate upgrade of time and timezone-series to fix the problem.

@ygale
Copy link
Owner

ygale commented Feb 25, 2018

@AshleyYakeley if you decide to go with (1) or (2) above, I'll try to provide a PR if you'd like.

@AshleyYakeley
Copy link

What about timezone-olson, is that also intended to be incorporated into time?

@ygale
Copy link
Owner

ygale commented Feb 25, 2018

In my opinion, timezone-olson still makes sense as a separate library. As parser/renderer of an external file format, it feels different to me than the time library. But up to you, you're welcome to incorporate that too if you'd like. I did try to keep to your coding style there too.

@ygale
Copy link
Owner

ygale commented Feb 25, 2018

And obviously, if we do incorporate any of my code into the time library, I am completely open to any changes you would like, including but not limited to shapes of types, names of functions and types, algorithms, overall structure, whatever. It's your bikeshed, not mine.

Altough it would be nice to provide a gentle migration path for current users of timezone-series if things do change. That migration path would likely be via one or more compatibility versions of timezone-series as a separate library.

@dbaynard
Copy link
Author

@ygale don't cut a new (broken) release on my account! In any case, unless there's some way you'd like me to help I won't be contributing to this issue further.

@ygale
Copy link
Owner

ygale commented Feb 25, 2018

@dbaynard no worries. Thanks for bringing this to our attention. It's important for everyone. We'll work together to get it right. If what we do doesn't work for you specifically, please let us know.

Actually, the best solution for you will probably be to upgrade both time and timezone-series, after we get this worked out. The reason I'm thinking of doing a partially broken release isn't for your use case. It's for people who are stuck on time 1.9 and can't upgrade or downgrade. E.g., if they are using stack and they must use a specific stackage release due to policy/integration reasons. Right now I'm thinking that I'll make an effort to help them by giving them a mostly-working solution, but I won't go all the way to suck in the entire FormatTime and ParseTime mechanism into timezone-series.

@dbaynard
Copy link
Author

I'd agree, but given time is a boot package most people won't upgrade beyond their compiler's version? At stackage both nightly and lts are on 1.8.0.2 even though 1.8.0.4 is out.

In any case, the approach seems sensible. I'm happy to have helped!

@AshleyYakeley
Copy link

Does timezone-series have any use other than for timezone-olson?

@ygale
Copy link
Owner

ygale commented Feb 25, 2018

That's by far the most common way and most important way to use it. I have some very old "to do" lists that also talk about:

  • Writing a parser for the timezone data found in the Windows registry
  • Writing a parser for the Eggert timezone text files that are the source code for Olson binary timezone files

It's also conceivable that you would generate your own TimeZoneSeries. For example, in simple situations for known timezones where that it is easier than reading Olson files. Or for various kinds of simulations.

EDIT: Currently you must generate your own TimeZoneSeries to use it into the future. A few years ago Olson files started supplying a POSIX timezone specification to extend its timezone series arbitrarily into the future. Another "to do" item is to support that in the timezone-series library.

@AshleyYakeley
Copy link

My current thinking:

  1. There's no point incorporating timezone-series without also incorporating timezone-olson.
  2. I should also incorporate my own timezone-unix, which uses timezone-olson.
  3. Support rules for transitions beyond the last explicitly listed one #8 and Time-zone conversions after 2037 wrong timezone-olson#3 need to be fixed.

@ygale
Copy link
Owner

ygale commented Feb 25, 2018

OK fine with me, let's shoot for that. Adding rules to the type will take work though. What approach should we take for avoiding immediate breakage? I'd like to put up a temporary version that compiles as quickly as possible. Preferrably before time >= 1.9 hits stackage nightlies, which will probably be very soon.

So I suppose I'll release the partially broken version, without the class instances, that compiles with current time 1.9, very soon.

Would you be willing to expose the class methods in an exported "Internal" module, at least temporarily? That way I can get a working version out soon afterwards. Or do I need to stay with the broken version until we are completely done?

@ygale
Copy link
Owner

ygale commented Feb 26, 2018

@dbaynard With modern build tools, there is no problem using a different version of the time library than the one that came with your GHC. As for stackage nightly still being on 1.8.* - that's right, it's a race against the - ahem - clock.

@AshleyYakeley
Copy link

AshleyYakeley commented Feb 26, 2018

I've created haskell/time#88 to expose the class methods. It's in the 1.9.1 milestone.

@AshleyYakeley
Copy link

I've created haskell/time#89 to provide an interface to the tz database, which requires functionality from timezone-series, timezone-olson, leapseconds, and timezone-unix. It's in the 1.10 milestone.

@AshleyYakeley
Copy link

OK, time-1.9.1 has been released, with module Data.Time.Format.Internal.

@ygale
Copy link
Owner

ygale commented Feb 28, 2018

@AshleyYakeley excellent thanks! I'm going to be out for a few days - hope to get to this early next week.

@mitchellwrosen
Copy link
Contributor

I made a patch for this in #10

@ygale
Copy link
Owner

ygale commented Mar 14, 2018

@mitchellwrosen Thanks! Merged.

@ygale ygale closed this as completed Mar 14, 2018
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