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

Unseal InjectK to allow for extension by other libraries #1642

Merged
merged 1 commit into from
May 9, 2017

Conversation

andyscott
Copy link
Contributor

@andyscott andyscott commented Apr 30, 2017

It would be extremely useful to open up the injection helpers in Free to support any target coproduct type. This is the simplest solution to address this "issue".

Related gitter conversation: https://gitter.im/typelevel/cats?at=590627c9d32c6f2f0956189d

For discussion:

  • Should this be converted to a trait?
  • Should apply/unapply be removed?
  • Should a test be added to ensure injection into type constructors other than EitherK can occur?

Copy link
Contributor

@edmundnoble edmundnoble left a comment

Choose a reason for hiding this comment

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

Looks good, I don't think practically sealing this buys us anything. This type class is much more useful as "injective natural transformations", rather than "how to get in and out of EitherK".

@andyscott
Copy link
Contributor Author

The only benefit, IMHO, of leaving it sealed is that the implementation stays true to the reference paper.

That being said, also I prefer that InjectK be a generalized injection type class for natural transformation.

@codecov-io
Copy link

codecov-io commented Apr 30, 2017

Codecov Report

Merging #1642 into master will not change coverage.
The diff coverage is 100%.

Impacted file tree graph

@@           Coverage Diff           @@
##           master    #1642   +/-   ##
=======================================
  Coverage   93.37%   93.37%           
=======================================
  Files         240      240           
  Lines        3941     3941           
  Branches      138      138           
=======================================
  Hits         3680     3680           
  Misses        261      261
Impacted Files Coverage Δ
core/src/main/scala/cats/InjectK.scala 100% <100%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5a90b61...11ddf9e. Read the comment docs.

@kailuowang kailuowang added this to the 1.0.0-MF milestone May 1, 2017
@kailuowang
Copy link
Contributor

kailuowang commented May 1, 2017

👍 for the change. Since we'd like to treat this as a typeclass, how about making it a trait? We may decide to add a SurjectK and BijectK later, BijectK can extend both SurjectK and InjectK.

Edit: another question: why not making apply and unapply final?

@andyscott
Copy link
Contributor Author

andyscott commented May 1, 2017

Update: I switched the class to a trait and added final to apply/unapply.

@kailuowang
Copy link
Contributor

Thanks very much @andyscott. It's probably too much to ask for this PR but I started to wonder if we should/can add laws to this new typeclass. We don't have typeclasses that take two type constructors yet, so it will be a first.

@andyscott
Copy link
Contributor Author

Adding laws seems reasonable. Would it be okay to handle that with a separate PR (and maybe an issue for discussion purposes)?

@kailuowang
Copy link
Contributor

do you want to create an issue for this law or I can create one? @andyscott

Copy link
Contributor

@kailuowang kailuowang left a comment

Choose a reason for hiding this comment

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

LGTM, I'll wait for another one or two signoff. @edmundnoble do you want to do another quick round?

Copy link
Contributor

@edmundnoble edmundnoble left a comment

Choose a reason for hiding this comment

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

I actually don't think we should change the "abstract class" part. Nobody should be using mixin inheritance with a class with an apply and an unapply; practically I don't believe anyone will, so the trait conceptual and bytecode overhead isn't worth it.

@kailuowang
Copy link
Contributor

kailuowang commented May 3, 2017

@edmundnoble I was thinking the possibility of having a SurjectK typeclass and BijectK typeclass mixing both SurjectK and InjectK (would BijectK be a natural isomorphism between F[_] and G[_]?).
The unapply is convenient but I feel that it should go to something like an InjectK syntax now that we treat it more like a typeclass? @sellout, since you added those two methods, I am curious to hear your opinion over this.
That said, I don't feel strongly about making it a trait, the BijectK and SurjectK maybe too long a shot to be a factor in this design.

@andyscott
Copy link
Contributor Author

andyscott commented May 3, 2017

I've changed it back to an abstract class.
RE: SurjectK-- I like it.
Also, I am bullish on splitting InjectK apart.

@kailuowang
Copy link
Contributor

@andyscott how do you split Inject apart? You mean taking the apply and unapply out or split inj and prj ?

@andyscott
Copy link
Contributor Author

@kailuowang I like the idea of splitting inj and prj apart, although I don't know if it has any practical benefit.

@kailuowang
Copy link
Contributor

kailuowang commented May 3, 2017

@andyscott it appears to me that you have to have them both available to write laws right?
Something like forall a, prj(inj(a)) == Some(a)

@sellout
Copy link
Contributor

sellout commented May 3, 2017

@kailuowang I would be fine with unapply moving to syntax, provided it’s still possible to use the unapply for pattern matching. We basically use it for peephole optimizations over a coproduct, so we often need to match a couple levels deep, and having to do that with match/prj/match/prj is painful and unreadable.

@andyscott
Copy link
Contributor Author

andyscott commented May 3, 2017

@kailuowang Yep, for any general roundtrip law(s) you need both.

@andyscott
Copy link
Contributor Author

andyscott commented May 5, 2017

@edmundnoble Are changes still needed? Should we move unapply to syntax?

@edmundnoble
Copy link
Contributor

@andyscott This gets a 👍 from me after the abstract class change.

@andyscott
Copy link
Contributor Author

@edmundnoble I've changed it back to abstract class already ☺️

@edmundnoble
Copy link
Contributor

I know. Sorry for the ambiguity ;)

@kailuowang
Copy link
Contributor

@andyscott thanks! Let's merge now and we can work on making it a type class for Functors later.

@kailuowang kailuowang merged commit 2db95a7 into typelevel:master May 9, 2017
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.

5 participants