Skip to content

Add various instances for NullOrUndefined #52

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.

Sign up for GitHub

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

Jump to bottom
Merged
merged 3 commits into from
Jan 29, 2017
Merged

Add various instances for NullOrUndefined #52

merged 3 commits into from
Jan 29, 2017

Conversation

@felixSchl
Copy link
Contributor

@felixSchl felixSchl commented Jan 12, 2017 •
edited
Loading

Add various useful instances for NullOrUndefined, Null and Undefined, such as Ord, Eq, Show and Newtype.

Fixes #51

paf31
paf31 suggested changes Jan 13, 2017
View reviewed changes
Copy link
Contributor

@paf31 paf31 left a comment

Choose a reason for hiding this comment

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

Could you please add these for the Null and Undefined types as well?

src/Data/Foreign/NullOrUndefined.purs Outdated
derive instance newtypeNullOrUndefined :: Newtype (NullOrUndefined a) _

instance showNullOrUndefined :: (Show a) => Show (NullOrUndefined a) where
show x = "NullOrUndefined " <> show (unwrap x)
Copy link
Contributor

@paf31 paf31 Jan 13, 2017

Choose a reason for hiding this comment

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

Could you please wrap this output in parens?

Copy link
Contributor Author

@felixSchl felixSchl Jan 14, 2017

Choose a reason for hiding this comment

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

Done for all instances, could you confirm it's correct?

src/Data/Foreign/NullOrUndefined.purs Outdated
instance showNullOrUndefined :: (Show a) => Show (NullOrUndefined a) where
show x = "NullOrUndefined " <> show (unwrap x)

instance eqNullOrUndefined :: (Eq a) => Eq (NullOrUndefined a) where
Copy link
Contributor

@paf31 paf31 Jan 13, 2017

Choose a reason for hiding this comment

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

You could just derive the Eq and Ord instances.

Copy link
Contributor Author

@felixSchl felixSchl Jan 14, 2017

Choose a reason for hiding this comment

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

Updated

@paf31
Copy link
Contributor

paf31 commented Jan 13, 2017

Thanks for working on this!

@felixSchl
Copy link
Contributor Author

felixSchl commented Jan 14, 2017

Ready for review

@paf31
Copy link
Contributor

paf31 commented Jan 14, 2017

Travis says

The import of module Data.Function is redundant

although I'm not sure why it's an error and not a warning.

Anyway, the code looks good, thanks! Could you please just fix up the imports?

@garyb
Copy link
Member

garyb commented Jan 14, 2017

although I'm not sure why it's an error and not a warning.

It's because we use psa in strict mode on the core libraries.

@paf31
Copy link
Contributor

paf31 commented Jan 14, 2017

👍 Ah, that sounds like a good idea, thanks.

@paf31
Copy link
Contributor

paf31 commented Jan 14, 2017

Thanks!

@paf31 paf31 merged commit 8aa66ac into purescript:master Jan 29, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer

@paf31 paf31 paf31 approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@felixSchl @paf31 @garyb