WIP: Month of year type #246
Conversation
| where | ||
| YearMonth y my = MkMonth $ (y * 12) + toInteger (pred $ clip 1 12 my) | ||
| YearMonth y my = MkMonth $ (y * 12) + toInteger (monthOfYearIndex (pred my)) | ||
|
|
||
| fromYearMonthValid :: Year -> MonthOfYear -> Maybe Month |
There was a problem hiding this comment.
I left this function for backward compatibility even though it is now no longer necessary.
|
|
||
| -- | The length of a given month in the Gregorian or Julian calendars. | ||
| -- First arg is leap year flag. | ||
| monthLength :: Bool -> MonthOfYear -> DayOfMonth | ||
| monthLength isLeap month' = monthLength' isLeap (clip 1 12 month') | ||
| monthLength isLeap month = case month of |
There was a problem hiding this comment.
This function may be slightly more performant now.
| instance Arbitrary MonthOfYear where | ||
| arbitrary = choose (1, 12) `suchThatMap` parseMonthOfYearIndex | ||
| shrink January = [] | ||
| shrink _ = [January] |
There was a problem hiding this comment.
More accurate shrinking may be in order.
|
#134 is already closed. |
|
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. |
|
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. |
|
Finally, this PR would break a lot of existing code. |
|
@AshleyYakeley you make an excellent point wrt breakage. I'd be happy to close this and make a new PR with only the following
That way backward incompatibility is minimised because Would such a PR be more acceptable? |
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.