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

Semigroup constraint for the class Action #55

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

AliceRixte
Copy link

I started to use monoid-extras, but one detail that stopped me : the Action class doesn't inherit of the semigroup structure of it's first argument. Is there a reason why the declaration of Action isn't

class Semigroup m => Action m s where

instead of the current

class Action m s where

I propose here a quick solution, which might break some code depending on Action. It seems to me that the necessary changes to the Product and Sum instances make much more sense like this , since it should be elements of the monoid Sum (or Product) that should act on their underlying set, and not the other way around.

Cheers !

@byorgey
Copy link
Member

byorgey commented Oct 24, 2023

Thanks for the PR! I will try to take a look at this soon.

@byorgey
Copy link
Member

byorgey commented Oct 24, 2023

This does make a lot of sense to me. I think probably the reason the Semigroup constraint was not there is that this library was initially created long ago, at a time when Semigroup was not a superclass of Monoid, so having a Semigroup constraint would have excluded types which were an instance of Monoid but not Semigroup. However, Semigroup has been a superclass of Monoid since GHC 8.4 (https://gitlab.haskell.org/ghc/ghc/-/wikis/proposal/semigroup-monoid) and we currently don't support any versions older than that, so we should be fine!

This does break the dual-tree package though it looks like for a simple and easily solved reason. I want to first make sure I can fix that and there are no other issues in the diagrams ecosystem; assuming that all works (as seems likely) we can then merge and release this.

@AliceRixte
Copy link
Author

That's great ! Don't hesitate if you need anything else to make this work :-)

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