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 instances for Data.Monoid #1101

Closed
wants to merge 6 commits into from

Conversation

AliceRixte
Copy link
Contributor

Most of Data.Monoid newtypes already had an instance. I added the following ones :

  • Product
  • Sum
  • All
  • Any


liftParseJSONList _ _ p a = coerce (p a)
Copy link
Collaborator

Choose a reason for hiding this comment

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

this removals are wrong.

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 added it back, and made it coherent with the rest of the code

Copy link
Contributor Author

@AliceRixte AliceRixte Jun 9, 2024

Choose a reason for hiding this comment

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

I also think it would make sense that these two lines

parseJSONList = liftParseJSONList omittedField parseJSON parseJSONList
omittedField = omittedField1

should be added to all the FromJSON instances from Data.Monoid. They were initially there in the First and Last and WrappedMonoid instances, but not in the Down instance. If you agree I can do this.

@phadej
Copy link
Collaborator

phadej commented Jun 9, 2024

To make this easier I made #1104

So now the instances are simply using DerivingVia and are a lot more trivial. Sorry for causing the conflict, but hopefully now your patch will be simpler to write.

@AliceRixte
Copy link
Contributor Author

Pretty neat ! It was simpler for me to create a new pull request rather than cleaning up this one. Thanks a lot !

@AliceRixte AliceRixte closed this Jun 11, 2024
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.

2 participants