-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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 FunctionN.liftN, parLiftN #4340
Conversation
I think I'd like to add the parallel versions too, but I don't want to invest additional time without some buy-in from the maintainers: let's talk about these first :) |
I like the idea. I don't think I like the tupledF name. I think we should iterate on that a bit. Maybe mapTuple? I don't know. |
I'm fine with pretty much any names ;) |
that's pretty cool, I didn't know about it. Frankly I'd still like to have something like this PR in cats because not everybody uses kittens, plus
|
Sure, I'm not arguing against this PR - just pointing out one could use kittens in the meantime. |
@joroKr21 That said,
They may both have merit, though: I don't have a strong opinion on whether |
Added |
Ah ok, the implementation is |
+1 on this to re-raise attention :) |
Removed |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM
merging with main apparently broke something, I'll take a look. |
I forgot to write down the failing hash and the logs are now gone :( |
Here is the log: https://github.com/typelevel/cats/actions/runs/8066611292/job/22035078228#step:13:7049
|
Can we revive this? The flaky test above doesn't seem to be related, and I find myself wanting this feature at least on a weekly basis. I think it'd be a very useful addition. |
Personally, I do like the idea, but still not sure that it belongs here: since it is a bunch of extension methods for standard Scala functions, it looks more like what mouse is for. We can put it straight into Cats for sure, but Mouse is a great library too and it would be definitely missing it ) |
I don't really agree that it's just a bunch of extensions for standard Scala functions: I would rather say it's alternative syntax for the existing I believe Foo(
name,
42
)
// almost the same
Foo.apply.liftN(
validateName,
42.validNel[String]
)
// feels upside down
(
validateName,
42.validNel[String]
).mapN(Foo.apply) so I don't think putting it in |
First of all, sorry if my "bunch" wording might look dismissively – I didn't mean that. I just meant that there are many of them, not just one. I see you point and it totally makes sense to me. For example, if class Foo private { ... } // cannot be instantiated with `new`
object Foo {
def create[F[_]](n: Int, s: String): F[Foo] = ???
}
def fetchNum: F[Int] = ???
def fetchStr: F[String] = ???
Foo.create.flatLiftN(fetchNum, fetchStr) // => F[Foo]
// instead of
(fetchNum, fetchStr).flatMapN(Foo.create) I have a feeling that this case may come along pretty soon. |
hmm I think It's interesting that the order of effects in For argument's sake, let's say we merge just |
Just for the record, we already have cats/core/src/main/scala/cats/Functor.scala Lines 76 to 89 in d1b0830
|
@armanbilge ,
I don't think such an import should ever be suggested in any of Cats examples ) UPD: #4659 |
@kubukoz , I think I'm fine with landing A couple more thoughts on the PR:
|
I think the variant with I'm adding the AnyVals now! |
@kubukoz ,
Oh, you're right, this line from my example compiles on Scala 2.13 with
It is just my IntelliJ that seems not recognizing such a syntax and marking it as an error. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thank you!
good old IntelliJ doing its thing... 😅 |
thanks for the merge, can't wait to see the release! |
liftN
is just likemapN
, but it should provide better type inference - if you have 6 effects to put in a tuple, you can make a mistake in one and suddenly the whole tuple no longer has amapN
methods. WithliftN
, you put the function first, and only then pass the (wrapped) elements.Consider this:
tupledF
is the same thing, but when your tuples are already in an effect - this can happen if you're unable to do.tupled
(e.g. combining cats-parse'sParser
+Parser0
- cc @johnynek) or you're just not in control over how the tuple is produced in the effect.tupledF
has been removed from this PR, we can revisit it in the future.