-
Notifications
You must be signed in to change notification settings - Fork 48
Added enum instance for Date and date implementation of adjust #73
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
Conversation
@LiamGoodacre Could you maybe have a look at this as well? Really think this is missing and since the constructors are hidden it is cumbersome to implement outside of the library, not to mention fairly complicated. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
@LiamGoodacre cleaned it up and avoided some code duplication by using the |
@LiamGoodacre Are we good to go? |
@garyb Could you have a look here? Should be ready to merge. |
src/Data/Date.purs
Outdated
@@ -69,6 +89,25 @@ weekday = unsafePartial \(Date y m d) -> | |||
let n = runFn3 calcWeekday y (fromEnum m) d | |||
in if n == 0 then fromJust (toEnum 7) else fromJust (toEnum n) | |||
|
|||
-- | Adjusts a date with a Duration in days. The day duration is | |||
-- | converted to an Int using fromNumber. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
My only comment in that case is let's explain the implication of fromNumber
directly here, rather sending people off to have to look up what it means.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm happy with this, thanks!
src/Data/Date.purs
Outdated
@@ -69,6 +89,26 @@ weekday = unsafePartial \(Date y m d) -> | |||
let n = runFn3 calcWeekday y (fromEnum m) d | |||
in if n == 0 then fromJust (toEnum 7) else fromJust (toEnum n) | |||
|
|||
-- | Adjusts a date with a Duration in days. The number of days must | |||
-- | fall within the valid range for an `Int` type otherwise `Nothing` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's be precise here and say:
The number must already be an integer and fall within the valid range of values for the Int type
because otherwise adjust (Days 3.1) someDate == Nothing
would be confusing
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I have added your suggestion
This adds:
I initially implemented adjust without adding an enum instance for date, but implementation became quite complicated although possibly more efficient since it can recur with the number of days in a month rather than recur day by day. An enum instance turns out to be very useful.