haskell / time

A time library

http://hackage.haskell.org/package/time

Other

118 stars 78 forks source link

WIP: Month of year type #246

Closed NorfairKing closed 9 months ago

NorfairKing commented 9 months ago

This PR exists to show that I'm happy to do the work necessary to resolve #134

Current status: The code compiles, but with warnings and failing tests.

At this point I want to acknowledge that resolving #134 is a nontrivial amount of work. The internals of the time library become slightly more complex in order to prevent footguns and runtime errors for users. There are also minor performance improvements in some places and regressions in others.

I understand that, as the maintainer @AshleyYakeley, you are in your right to close this PR without comment or even ignore it entirely. However, I would strongly urge you to consider how many footguns and runtime issues this change can prevent, in the interest of the library's users.

I also understand that this PR will likely cause some ruffled feathers, so I want to make it extra clear that I mean no offense toward anyone who disagrees with my technical viewpoint on this matter. I only want to make an effort towards a less dangerous technical solution.

AshleyYakeley commented 9 months ago

134 is already closed.

AshleyYakeley commented 9 months ago

This PR would make it easier to work with months-of-year as names, while more difficult to work with them as numbers. I believe the latter is the more common case, so a net negative.

AshleyYakeley commented 9 months ago

The "footgun" in the current design (using a month-of-year outside the range 1..12) is already obvious to anyone using the library, and is mirrored by "footguns" for day-of-month, hour-of-day, etc., which this PR does not (and of course should not) address.

AshleyYakeley commented 9 months ago

Finally, this PR would break a lot of existing code.

NorfairKing commented 9 months ago

@AshleyYakeley you make an excellent point wrt breakage.

I'd be happy to close this and make a new PR with only the following

Turn all occurrences of the MonthOfYear synonym back into Int
Add the parseMonthOfYearIndex and monthOfYearIndex functions.
Make a new MonthOfYear sum type

That way backward incompatibility is minimised because fromGregorian/toGregorian remain unchanged, and users don't need to define their own MonthOfYear type that then clashes in the namespace with the type synonym.

Would such a PR be more acceptable?