Skip to content
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

ADD: Eq, Ord, and Show instances for JSDate #17

Merged
merged 9 commits into from
Mar 20, 2018

Conversation

eric-corumdigital
Copy link
Contributor

No description provided.

@eric-corumdigital
Copy link
Contributor Author

Resolves #14

@@ -263,3 +272,6 @@ toTimeString dt = runFn2 dateMethod "toTimeString" dt
-- | Returns the date as a string using the UTC timezone.
toUTCString :: JSDate -> String
toUTCString dt = runFn2 dateMethod "toUTCString" dt

-- | Returns the date at a number of milliseconds since 1970-01-01 00:00:00 UTC.
foreign import fromTime :: Number -> JSDate
Copy link
Member

Choose a reason for hiding this comment

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

fromTime isn't exported or used?

Copy link
Member

Choose a reason for hiding this comment

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

It already exists as fromInstant too, I'd prefer we use that instead.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

fromTime copes with NaN and out of bound Numbers in the native way, which is not something that makes much sense with Instant.

Copy link
Member

Choose a reason for hiding this comment

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

Can you add the fromTime export? Then I'll finally get this merged in and released.

Copy link
Member

Choose a reason for hiding this comment

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

I guess it disappeared again in the convoluted reversion process 😉

@eric-corumdigital
Copy link
Contributor Author

@LiamGoodacre I have not the slightest idea of what you are talking about.

@garyb
Copy link
Member

garyb commented Mar 19, 2018

Generic instances aren't suitable for JSDate - Generic can only accurately represent values that are PS sum or product types.

@eric-corumdigital
Copy link
Contributor Author

eric-corumdigital commented Mar 20, 2018

I agree that this is not an obvious use of Generic but I do not know what the problem is. My understanding of Generic is that it supports encoding and decoding from types constructed from select constructors such as Sum, Argument, and so forth. The choice of these type constructors is clearly to mimic the grammar of ADT definitions so use in that case becomes obvious.

The ADT JSDate is analogous to is:

newtype JSDate = JSDate Number

Hence my choice for the Generic representation. Number is an accurate representaton of JSDate because fromTime <<< getTime = id.

@garyb
Copy link
Member

garyb commented Mar 20, 2018

Well, as an example, genericShow is incorrect with this instance. Generic is not meant to be used for foreign types.

@eric-corumdigital
Copy link
Contributor Author

I am not invested in the intentions of Generic. I am just looking at what Generic actually is.

There is no reason to expect show = genericShow but lets say it is expected that, like show, genericShow should be valid PureScript code to reconstruct the value. In this case the answer is simply to change the constructor name from "JSDate" to "fromTime".

> genericShow (fromTime 0.0)
"(fromTime 0.0)"

I will submit that revision.

genericShow produces valid PureScript code.
@garyb
Copy link
Member

garyb commented Mar 20, 2018

To be completely upfront (and well, a more than a little stubborn) I'm probably not going to merge this as long as there's a Generic instance.

Can I ask why the instance is even useful for this type, since you've implemented the other instances that are usually generic derived anyway?

@eric-corumdigital
Copy link
Contributor Author

No I am glad you are making the discussion straight-forward! Okay, I have reverted the addition of Generics.

@garyb garyb merged commit 54a32b6 into purescript-contrib:master Mar 20, 2018
@thomashoneyman thomashoneyman mentioned this pull request Sep 12, 2020
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants