Skip to content

Conversation

@imarios
Copy link
Contributor

@imarios imarios commented Jan 15, 2018

Just realized that these basic methods are missing. You can always get the by resulting back to .dataset, but this way is much easier.

@codecov-io
Copy link

codecov-io commented Jan 15, 2018

Codecov Report

Merging #230 into master will decrease coverage by 0.04%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master     #230      +/-   ##
==========================================
- Coverage   96.66%   96.62%   -0.05%     
==========================================
  Files          52       52              
  Lines         960      859     -101     
  Branches       13       10       -3     
==========================================
- Hits          928      830      -98     
+ Misses         32       29       -3
Impacted Files Coverage Δ
...ataset/src/main/scala/frameless/TypedDataset.scala 90.62% <100%> (-4.87%) ⬇️

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 33a39c6...01c0e5c. Read the comment docs.

@imarios
Copy link
Contributor Author

imarios commented Jan 16, 2018

@OlivierBlanvillain this should be a pretty easy review. Give it a look when you have any time.

@OlivierBlanvillain
Copy link
Contributor

These where voluntarily left out, first and head are unsafe (you should use .firstOption.get instead), and head(n) is already implemented as def take(num: Int)(implicit F: SparkDelay[F]): F[Seq[T]].

In general, I really dislike having two APIs for the same thing (first/head, where/filter and so on), so the approach so far has been to include only one variation, but that's maybe something to reconsider...

@imarios
Copy link
Contributor Author

imarios commented Jan 19, 2018

Thank you @OlivierBlanvillain, yea, now it's clear to me. It was just one night me trying to write a frameless tutoria and realized head(n) is missing. We have firstOption and take(n) that do exactly what I needed and are safe and wrap the effect correctly.

However, if even me, that wrote some parts of the library hit a (temporary) wall that prevents a direct translation of my familiarity with the vanilla Spark APIs into frameless, it makes me wonder if other, casual users, have the same experience. In some way, I am a a head and head(n) guy and not a first and take(n) guy.

Clearly, adding the APIs the way are coded this PR is wrong, so no question there. However, I am really entertaining the idea of adding headOption and head(n) to be aliases to firstOption and take(n) respectively. Just for the sake of familiarity.

@OlivierBlanvillain
Copy link
Contributor

Clearly, adding the APIs the way are coded this PR is wrong, so no question there. However, I am really entertaining the idea of adding headOption and head(n) to be aliases to firstOption and take(n) respectively. Just for the sake of familiarity.

Sounds good to me, we could even add @deprecated head and first methods, just for discoverability.

/** Alias for [[firstOption()]].
*/
@deprecated("Use headOption or firstOption instead.", "0.5.0")
def head[F[_]]()(implicit F: SparkDelay[F]): F[Option[T]] = firstOption()
Copy link
Contributor

Choose a reason for hiding this comment

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

I would match the vanilla signature to simplify migration. Deprecation is already harsh enough 😄

Copy link
Contributor Author

@imarios imarios Jan 21, 2018

Choose a reason for hiding this comment

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

I though giving them the same method name was enough :D. Let me resolve the conflicts on this one after merging #153. I can make them just forward the result to vanilla head and first.

@imarios imarios merged commit f71e0a1 into typelevel:master Jan 27, 2018
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.

3 participants