Skip to content
This repository was archived by the owner on Oct 4, 2020. It is now read-only.

Implement with instance chains #10

Closed
wants to merge 1 commit into from
Closed

Implement with instance chains #10

wants to merge 1 commit into from

Conversation

garyb
Copy link
Member

@garyb garyb commented Apr 13, 2018

I think as per #9 we probably want to deprecate this library and put these classes with Either/Coproduct, but just thought I'd try this out.

@LiamGoodacre I was a little surprised to find that it didn't raise an error even when these were defined without instance chains until used. I thought purescript/purescript#3129 would have changed that? Maybe I misunderstood.

@LiamGoodacre
Copy link

Ooooh, I will have a play tomorrow and see if I messed something up. Thanks!

@@ -0,0 +1,23 @@
module Data.Inject.Either where

Choose a reason for hiding this comment

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

Should be Data.Inject.Coproduct?

Copy link
Member Author

Choose a reason for hiding this comment

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

Uh yes 😉

Copy link

Choose a reason for hiding this comment

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

Yeah, this change is required to compile - it collides with the other Data.Inject.Either module in this project.

@LiamGoodacre
Copy link

Are you sure you were building with latest master?
Without instance chains I get:

    Overlapping type class instances found for

      Main.Inject f
                  (Coproduct f g)

    The following instances were found:

      Main.injectReflexive
      Main.injectLeft

@LiamGoodacre
Copy link

(The error says Main. because I copied the definitions into an example project.)

@garyb
Copy link
Member Author

garyb commented Apr 14, 2018

Ack, maybe it was user error on my part then... I did pull master and stack install for sure, but maybe I still did something wrong in there / it was using the wrong PATH or something.

The other thing I thought of, is this version using instance chains can inject into the left or the right, the original definition is biased to one side, so perhaps I tested with that and it failed. I'll try and reproduce again later today, sorry to waste your time!

@garyb
Copy link
Member Author

garyb commented Apr 14, 2018

@LiamGoodacre I figured it out. I did git pull master but not git pull purescript master 🤦‍♂️

Copy link

@chexxor chexxor left a comment

Choose a reason for hiding this comment

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

Requires a few changes to compile with the 0.12 package-set.

prj = coproduct (const Nothing) prj

else instance injectReflexive :: Inject f f where
inj = id
Copy link

Choose a reason for hiding this comment

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

id => identity

@@ -0,0 +1,23 @@
module Data.Inject.Either where
Copy link

Choose a reason for hiding this comment

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

Yeah, this change is required to compile - it collides with the other Data.Inject.Either module in this project.

prj = either (const Nothing) prj

else instance injectReflexive :: Inject a a where
inj = id
Copy link

Choose a reason for hiding this comment

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

id => identity

@chexxor
Copy link

chexxor commented Apr 29, 2018

Requested changes are addressed in #11, so you can merge that, then merge this PR later.

@LiamGoodacre
Copy link

Closing as we will move these type classes and instances to functors/either.

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants