haskell / time

A time library
http://hackage.haskell.org/package/time
Other
118 stars 78 forks source link

[Controversial] Problems of `Read` instance of `TimeZone` #203

Closed SomeAcnt closed 2 years ago

SomeAcnt commented 2 years ago

Hello, I and couple others noticed that Read instance of TimeZone type is not only unlawful, but also covers only handful amount of timezones. I understand that it is hard enough to include all possible timezones, so imo removing Read instance would be desirable. However, it seems like in https://github.com/haskell/time/issues/28, the rationale to keep Read instance is not well-explained.

Let me point this out, that Read instance has several problems:

If this were to be fixed, these are possible suggestions:

  1. Avoids breaking changes: For unknown timezones only, change Show instance of TimeZone to emit the UTC offset & timeZoneSummerOnly field as well. Read instances could recognize these for unknown timezones.
  2. Embraces breaking changes: Remove Read instance altogether, as it is potentially confusing and only works for handful of cases.

If not, could you provide more detailed rationale on why Read instance should be kept as is?

Sincerely, SomeAcnt

AshleyYakeley commented 2 years ago

Surely changing the Show instance is a breaking change?

SomeAcnt commented 2 years ago

Well I meant to say less breaking change - you can mark it as breaking change. On the other hand, I think it is reasonable to expect Show instance for unknown timezones would not be relied upon, and instead would be used for debugging purposes. I don't have concrete numbers, but it is already quite dangerous to rely on format of Show of TimeZone on serious applications.

Another possibility is to let TimeZone as is, and only change Read/Show instances for ZonedTime.

AshleyYakeley commented 2 years ago

See #191 for ZonedTime.

AshleyYakeley commented 2 years ago

The Read and Show instances for TimeZone match RFC 822 sec 5.1. This is why they don't contain all the information, and why those particular time-zones are recognised.

SomeAcnt commented 2 years ago

I did not know that it only lists timezones of US. How about changing documentation of the Read instance to point the RFC822, then? It is not easy to see the rationale for now.