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 implementation for Can #109

Open
wants to merge 35 commits into
base: main
Choose a base branch
from
Open

Add implementation for Can #109

wants to merge 35 commits into from

Conversation

pablisco
Copy link
Contributor

@pablisco pablisco commented Jun 7, 2020

Transferred from arrow-kt/arrow-core#103

Implementation of Canfrom the Haskell Smash package: https://hackage.haskell.org/package/smash-0.1.1.0/docs/Data-Can.html

Related to #108

@pablisco pablisco changed the title Add smash module with can type Add can type Jun 7, 2020
@pablisco pablisco changed the title Add can type Add implementation for the Can type Jun 8, 2020
Copy link
Member

@aballano aballano left a comment

Choose a reason for hiding this comment

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

Looking good! Some quick comments

arrow-mtl-data/src/main/kotlin/arrow/mtl/Can.kt Outdated Show resolved Hide resolved
arrow-mtl-data/src/main/kotlin/arrow/mtl/Can.kt Outdated Show resolved Hide resolved
arrow-mtl-data/src/main/kotlin/arrow/mtl/Can.kt Outdated Show resolved Hide resolved
@aballano aballano requested a review from pakoito June 9, 2020 13:00
@pablisco pablisco changed the title Add implementation for the Can type Add implementation for Can Jun 9, 2020
SGR: Semigroup<R>,
b: Can<L, R>
): Can<L, R> = when (val a = this) {
is Neither -> when (b) {
Copy link
Member

Choose a reason for hiding this comment

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

[WTB] Pattern matching

Copy link
Member

@pakoito pakoito 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'd move some of the implementations to the datatype

@pablisco
Copy link
Contributor Author

pablisco commented Jul 19, 2020

Are we good with this? @aballano :)

Copy link
Member

@raulraja raulraja left a comment

Choose a reason for hiding this comment

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

Thanks @pablisco; @1Jajen1 may be interested in reviewing as @nomisRev pointed out since he is our most active MTL contributor. Also this needs docs for the site and docs. As we approach v1 we can just drop undocumented stuff in the public API since its a maintenance burden to explain individually what this does.

@1Jajen1
Copy link
Member

1Jajen1 commented Jul 21, 2020

I'll look at this later today :)

Did a quick skim right now and this seems really well done 👍

However I do have one question:This is not a transformer, why would we put this into mtl? I can understand keeping arrow-core slim, but mtl does not seem like the right place for this. I don't have a much better idea where to put it though, so it might be fine for now.

Copy link
Member

@1Jajen1 1Jajen1 left a comment

Choose a reason for hiding this comment

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

All in all this is really well done, i have just a few suggestions on the docs part (take or leave, it's mostly subjective anyway) and some other nits. I also think the monoid instance is broken atm and the property test generator could be improved.

As Raul said this needs an entry in the docs. You should be able to link the apidocs in the sidebar menu and that should be sufficient.

And lastly: This should not be in mtl. This is not a transformer, nor has it any dependency on any incubator code (or mtl specific code). If we don't want this in core we should at least discuss how to handle these datatypes and typeclasses. (In general there are some useful types and classes that don't really belong in core, but could be useful to offer in other libs.

Going forward with this: How should we continue here? @pakoito @raulraja @aballano

  • Keep it around mtl for now and organize later?
  • Merge it into core and split core later?
  • Create something new now? (I'd hate to do this to this pr as it is close to complete, so one of the above is probably better)

arrow-mtl-data/src/main/kotlin/arrow/mtl/Can.kt Outdated Show resolved Hide resolved
arrow-mtl-data/src/main/kotlin/arrow/mtl/Can.kt Outdated Show resolved Hide resolved
arrow-mtl-data/src/main/kotlin/arrow/mtl/Can.kt Outdated Show resolved Hide resolved
arrow-mtl-data/src/main/kotlin/arrow/mtl/Can.kt Outdated Show resolved Hide resolved
arrow-mtl/src/main/kotlin/arrow/mtl/extensions/can.kt Outdated Show resolved Hide resolved
arrow-mtl/src/main/kotlin/arrow/mtl/extensions/can.kt Outdated Show resolved Hide resolved
@raulraja
Copy link
Member

IMO A new module called experimental can be added to the incubator with sub modules if needed to group topics. MTL is still in incubator because we are close to find a encoding that does not require Kinds for polymorphism and can interleave effects in suspension. I believe all these transformers could be rewritten that way and the types would be much easier without kinds.

@1Jajen1
Copy link
Member

1Jajen1 commented Jul 22, 2020

IMO A new module called experimental can be added to the incubator with sub modules if needed to group topics

That does make sense to me as well. We could also add something like arrow-extras as a repo and get individual modules there for the more obscure and rare use types and typeclasses.

MTL is still in incubator because we are close to find a encoding that does not require Kinds for polymorphism and can interleave effects in suspension. I believe all these transformers could be rewritten that way and the types would be much easier without kinds.

I'd love to drop mtl as soon as we have an actual effect system, do you have some links for this? I'd love to read up on it's attempted in arrow.

pablisco and others added 11 commits July 22, 2020 18:09
Co-authored-by: Jannis <overesch.jannis@gmail.com>
Co-authored-by: Jannis <overesch.jannis@gmail.com>
Co-authored-by: Jannis <overesch.jannis@gmail.com>
Co-authored-by: Jannis <overesch.jannis@gmail.com>
Co-authored-by: Jannis <overesch.jannis@gmail.com>
Co-authored-by: Jannis <overesch.jannis@gmail.com>
Co-authored-by: Jannis <overesch.jannis@gmail.com>
Co-authored-by: Jannis <overesch.jannis@gmail.com>
Co-authored-by: Jannis <overesch.jannis@gmail.com>
@pablisco
Copy link
Contributor Author

Thank you @1Jajen1 for all the comments 🙇 I'll try to get through them over the weekend :)

Btw, I'm happy to move Can somewhere more pertinent :)

Copy link
Member

@1Jajen1 1Jajen1 left a comment

Choose a reason for hiding this comment

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

Looking good!

So apart from figuring out where this is supposed to go this pr looks ready to merge.

Can.fromOptions(it.first, it.second)
}
}
private fun <A, B, R> Gen<A>.alignWith(genB: Gen<B>, transform: (Ior<A, B>) -> R): Gen<R> =
Copy link
Member

Choose a reason for hiding this comment

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

This is actually quite nice 👍 Might be possible to even define an instance for semialign on generators! (takes notes for arrow-check... 😅)

With kotest those will likely stay unlawful so this is perfectly fine!

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 can see GenK brewing here 😄

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.

None yet

6 participants