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

Separate left and right actions #38

Open
wants to merge 3 commits into
base: master
Choose a base branch
from

Conversation

madgen
Copy link

@madgen madgen commented Dec 26, 2018

Separates left and right actions as discussed in #37

  • Moves contents of Action to Action.LeftAction
  • Aliases Action and act to LeftAction and leftAct
  • Adds Action.RightAction analogous to Action.LeftAction

Hope this is sensible. Let me know if any changes needed.

Also happy holidays!

Mistral Contrastin added 3 commits December 26, 2018 00:58
The Action typeclass really represents left actions. This moves Action
under an Action namespace with the name LeftAction. It also updates
files where Action instances are defined and make them use LeftActions.

It also creates a type alias Action in the old Action namespace pointing
to LeftAction and act pointing to leftAct for compatibility.
Pretty much mirrors LeftAction.
@byorgey
Copy link
Member

byorgey commented Jan 13, 2019

Thanks! Sorry for the delayed response, I will take a look soon.

Copy link
Member

@byorgey byorgey left a comment

Choose a reason for hiding this comment

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

After looking this over my biggest complaint is that leaving in Action and act as aliases for LeftAction and leftAct does not actually prevent breakage: it is still going to break any existing instances of Action, which will have to be changed to LeftAction. (While it is possible to make an instance for a type synonym, it is not possible to make an instance of a synonym (I tried)---e.g. if we have class Foo and then type Bar = Foo, it is not possible to say instance Bar T where ....)

A proposed compromise is to simply leave the Action class alone and add RightAction as a new class. This has the benefit of being a much simpler change and not breaking any existing code. One might complain that it is asymmetric, but I think the asymmetry is sensible here --- left actions are much more common/natural than right actions because of the asymmetry in the way we write function application and composition. What do you think?

-- values of type @m@ act (@rightAct@) on values of another type @s@.
-- Instances are required to satisfy the laws
--
-- * @mempty \`rightAct\` s = s@
Copy link
Member

Choose a reason for hiding this comment

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

These laws need to be fixed, I think they are copy-pasted from the left action laws.

-- has chosen to have instance selection for @RightAction@ driven by the
-- /first/ type parameter. That is, you should never write an
-- instance of the form @RightAction m SomeType@ since it will overlap
-- with instances of the form @Action SomeMonoid t@. Newtype
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
-- with instances of the form @Action SomeMonoid t@. Newtype
-- with instances of the form @RightAction SomeMonoid t@. Newtype

-- act individually.
instance (Action m r, Action n r) => Action (m :+: n) r where
act = appEndo . mconcat . map (Endo . either act act) . unMCo
-- | Coproducts left actions on other things by having each of the components
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
-- | Coproducts left actions on other things by having each of the components
-- | Coproducts act on other things by having each of the components

instance (Action m r, Action n r) => Action (m :+: n) r where
act = appEndo . mconcat . map (Endo . either act act) . unMCo
-- | Coproducts left actions on other things by having each of the components
-- left actions individually.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
-- left actions individually.
-- act individually.

@madgen
Copy link
Author

madgen commented Jan 21, 2019

@byorgey that sounds fair to me. It happens to be a super busy week for me. So I won't be able to get to this until the weekend.

Just to be clear basically what needs to be done boils down to reverting 2cead61 and fixing the bits of documentation that I screwed up in RightAction.hs. Is that correct?

@byorgey
Copy link
Member

byorgey commented Jan 21, 2019

@byorgey that sounds fair to me. It happens to be a super busy week for me. So I won't be able to get to this until the weekend.

OK, given how long it took me to respond, obviously there's no rush. =)

Just to be clear basically what needs to be done boils down to reverting 2cead61 and fixing the bits of documentation that I screwed up in RightAction.hs. Is that correct?

Yes, that sounds right to me.

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