Skip to content

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

Merged
merged 6 commits into from
Oct 25, 2018
Merged

Added enum instance for Date and date implementation of adjust #73

merged 6 commits into from
Oct 25, 2018

Conversation

mschristiansen
Copy link
Contributor

This adds:

  • instance enumDate :: Enum Date
  • adjust :: Days -> Date -> Maybe Date
  • tests covering positive and negative day duration and large enough to test across leap years

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.

@mschristiansen mschristiansen mentioned this pull request Aug 16, 2018
@mschristiansen
Copy link
Contributor Author

@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.

Copy link
Member

@LiamGoodacre LiamGoodacre left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks!

@mschristiansen
Copy link
Contributor Author

@LiamGoodacre cleaned it up and avoided some code duplication by using the enum instances. Should be ready.

@mschristiansen
Copy link
Contributor Author

@LiamGoodacre Are we good to go?

@mschristiansen
Copy link
Contributor Author

@garyb Could you have a look here? Should be ready to merge.

@@ -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.
Copy link
Member

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.

Copy link
Member

@LiamGoodacre LiamGoodacre left a 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!

@@ -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`
Copy link
Member

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

Copy link
Contributor Author

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

@kritzcreek kritzcreek merged commit 525691e into purescript:master Oct 25, 2018
@mschristiansen mschristiansen deleted the enum branch October 25, 2018 20:02
@mschristiansen
Copy link
Contributor Author

Closes #44 and #72 and all the current pull requests

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

Successfully merging this pull request may close these issues.

4 participants