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

Adding Id.apply #4036

Merged
merged 1 commit into from
Nov 6, 2021
Merged

Adding Id.apply #4036

merged 1 commit into from
Nov 6, 2021

Conversation

BalmungSan
Copy link
Contributor

I hope there isn't a reason for not doing this.

@joroKr21
Copy link
Member

joroKr21 commented Nov 4, 2021

You could just do a: Id[A]

@BalmungSan
Copy link
Contributor Author

@joroKr21 sure, but a lot of times I have wanted to use Id to then use map as a pipe of pure functions to pure values. But then if I have to do (foo: Id[Foo]).map(f) looks worse and usually breaks this chain idea I had in mind in the first place.
The idea of this PR is to allow us to do Id(foo).map(f)

@satorg
Copy link
Contributor

satorg commented Nov 5, 2021

@BalmungSan another way with help of the existing Cats syntax could be a.pure[Id]:

import cats._
import cats.syntax.all._

val a = 123
println(a.pure[Id].map(_ + 456))

https://scastie.scala-lang.org/satorg/Qq3pz0kaT8CLJqfTye2VBA/17

@joroKr21
Copy link
Member

joroKr21 commented Nov 5, 2021

Since Scala 2.13 there is also a standard library pipe syntax in scala.util.chaining btw

@BalmungSan
Copy link
Contributor Author

@satorg oh nice, I kind of always forget about it.
@joroKr21 right, I personally don't like the names the used but yeah that is a valid point.

I guess the point of the PR is not really if this is the best way or not to access it. But, rather why not having the Id object?
Even more, I think this could serve as the initial step of a bigger change of moving Id into cats.data and moving the typeclass instances of Id into its companion object. The problem is that such changes are source and binary breaking so they must wait and I am not sure if everyone agree if those make sense.

@joroKr21
Copy link
Member

joroKr21 commented Nov 5, 2021

Yeah it's definitely a valid point to discuss on it's own merits whether Id.apply should exist or not.
I'm just giving you some alternatives you can apply today.

Regarding the type class instances, that wouldn't work in Scala 2 anyway because type aliases can't have a companion object. I.e. you can have an object Id, it's just not a companion. A completely separate question is if Id should be an opaque type in a hypothetical Scala 3 only version of Cats.

tpolecat
tpolecat previously approved these changes Nov 5, 2021
Copy link
Member

@tpolecat tpolecat left a comment

Choose a reason for hiding this comment

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

I like this change. It's trivial and it provides an ergonomic improvement that I would use from time to time.

@satorg
Copy link
Contributor

satorg commented Nov 5, 2021

I personally don't mind this change. Just to note: there's IdSuite as well. I wonder would it make sense to put a small test for this change there? Something like

property("Id#apply") {
  forAll { (a: Int) =>
    assert(Id(a) == (a: Id[Int])) // or `== a.pure[Id]` or even just `== a`
  }
}

@BalmungSan
Copy link
Contributor Author

Uhm that error is not related to my change, is it?

@tpolecat
Copy link
Member

tpolecat commented Nov 5, 2021

I restarted the build.

Copy link
Member

@rossabaker rossabaker left a comment

Choose a reason for hiding this comment

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

There are other ways, but I've looked for this way before. It's intuitive to me.

I wish it lived in cats.data, but it's not easy to get there from here in Cats 2.

@rossabaker rossabaker merged commit 6a6d4e4 into typelevel:main Nov 6, 2021
@BalmungSan BalmungSan deleted the patch-1 branch November 6, 2021 19:57
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

5 participants