search

twitter / scalding

A Scala API for Cascading
http://twitter.com/scalding
Apache License 2.0
3.5k stars 706 forks source link

Move all TypedPipe keyed functions to Keyed #1773

Closed johnynek closed 6 years ago

johnynek commented 6 years ago

closes #1733

I'm very happy with this. Note that in the tests only 1 place did we have to change some type parameters. However, with this change type-inference on mapValues, flatMapValues and eitherValues should actually work correctly. That's a nice win.

It also removes a lot of junky T <:< (K, V) and finally it centralizes all the keyed methods in one place. I am more and more happy about this change and I think it is a real win.

cc @fwbrasil @avibryant @ianoc

ianoc commented 6 years ago

This looks good to me -- should we have any concern about intellij users? i know usage of implicits can make them sad sometimes (those random toTypedPipe i see in codebases...).

Spark has this pattern for its keyed operations so might be able to ignore the ^^ given that its already pretty common?

I suspect somewhere in one of the main code bases something will get confused -- and as you had it'll require some changes if anyone has explicitly provided the implicit args to the group methods. But ¯_(ツ)_/¯ don't think thats a huge deal.

I'm personally 👍 and bullish. No changes needed I can see. Not sure if twitter folks have any fud tho since I expect they will have manual changes in a few places

non commented 6 years ago

👍 from me too

johnynek commented 6 years ago

I've confirmed with @gkk-stripe our local intellij expert that this PR should not hamper intellij users.

Given that this will improve the type inference (since it forces scala to first fix K, V), I think this will be a usability win.