-
Notifications
You must be signed in to change notification settings - Fork 178
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
NFData1,NFData2 instances (#767) #992
base: master
Are you sure you want to change the base?
Conversation
This would be good to have. @dbeacham would you like to finish this PR? |
Happy to - I'll finish it up next week. |
83e4566
to
57cc685
Compare
cdb3a07
to
6ef991c
Compare
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.
You could try the foldMap
thing if you like, if not this looks good to me.
containers/changelog.md
Outdated
* `NFData1`, `NFData2` instances for `Data.Graph`, `Data.IntMap`, | ||
`Data.IntSet`, `Data.Map`, `Data.Sequence`, `Data.Set`, `Data.Tree` and |
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.
Data.Graph
could be misunderstood, SCC
got the instance (not Graph
). And IntSet
is not affected.
We can also just drop the Data.
and say "...for IntMap
, Map
, Set
, Tree
, SCC
", I think that's good enough.
Speaking of changes, we usually use |
@dbeacham could you please correct the changelog so we can merge this? Also it needs a rebase. |
6ef991c
to
a014c1d
Compare
That's done. I left the |
There's a conflict preventing merge, please rebase on latest master. |
I've added
NFData1
andNFData2
instances where appropriate forData.Graph
,Data.IntMap
,Data.IntSet
,Data.Map
,Data.Sequence
,Data.Set
,Data.Tree
and relevant internal dependenciesTODO:
@since
docs