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

fix Task scaladocs #718

Merged
merged 1 commit into from Sep 19, 2018

Conversation

Projects
None yet
3 participants
@jozic
Contributor

jozic commented Sep 11, 2018

Fixes all scaladocs for Task, thus completing fixing scaladocs for monix-eval project

@oleg-py @alexandru please review

@oleg-py

I'm very happy to have your PRs coming. Makes my mornings much more enjoyable 👍

@@ -132,7 +135,7 @@ import scala.util.{Failure, Success, Try}
* But here's something else that the `Future` data type cannot do:
*
* {{{
* task.memoizeOnSuccess
* Task.eval(println("moo")).memoizeOnSuccess

This comment has been minimized.

@oleg-py

oleg-py Sep 11, 2018

Collaborator

I think this sample is worth adding random error throwing, e.g.:

Task.eval {
  println("moo")
  if (scala.util.Random.nextDouble() >0.33) throw new RuntimeException()
}
@@ -1984,6 +2021,7 @@ object Task extends TaskInstancesLevel1 {
* This is a alias for:
*
* {{{
* val thunk = () => 42

This comment has been minimized.

@oleg-py

oleg-py Sep 11, 2018

Collaborator

The equivalence holds with def thunk = 42, or Task.eval(thunk()), but not quite with your example

@@ -2282,7 +2329,7 @@ object Task extends TaskInstancesLevel1 {
* Based on Phil Freeman's
* [[http://functorial.com/stack-safety-for-free/index.pdf Stack Safety for Free]].
*/
def tailRecM[A,B](a: A)(f: A => Task[Either[A,B]]): Task[B] =
def tailRecM[A, B](a: A)(f: A => Task[Either[A, B]]): Task[B] =

This comment has been minimized.

@oleg-py

oleg-py Sep 11, 2018

Collaborator

@alexandru maybe we should just extends StackSafeMonad instead of having this method?

@@ -2795,7 +2853,7 @@ object Task extends TaskInstancesLevel1 {
* that signals a result.
*
* {{{
* val list: List[Task[Int]] = List(t1, t2, t3, ???)
* val list: List[Task[Int]] = List(Task(10), Task(20), Task(30))

This comment has been minimized.

@oleg-py

oleg-py Sep 11, 2018

Collaborator

A better example would be something along the lines of Task.sleep(x.second).map(_ => y)

@@ -2819,8 +2877,8 @@ object Task extends TaskInstancesLevel1 {
* this being equivalent with plain [[race]]:
*
* {{{
* val ta: Task[A] = ???
* val tb: Task[B] = ???
* val ta = Task("a")

This comment has been minimized.

@oleg-py

oleg-py Sep 11, 2018

Collaborator

See my comment above regarding using sleep

* (taskA, taskB, taskC).parMap { (a, b, c) =>
* val taskA = Task("a")
* val taskB = Task("b")
* val taskC = Task("c")

This comment has been minimized.

@oleg-py

oleg-py Sep 11, 2018

Collaborator

See my comment above regarding using sleep. Also maybe add a note that it would complete in about 3 seconds, if 3 is your longest delay.

@jozic jozic force-pushed the jozic:fix-task-scaladocs branch from f2730b3 to fac67b1 Sep 14, 2018

@jozic

This comment has been minimized.

Contributor

jozic commented Sep 14, 2018

Thank you @oleg-py for your kind and encouraging words!
i've addressed your comments.

@codecov

This comment has been minimized.

codecov bot commented Sep 14, 2018

Codecov Report

Merging #718 into master will increase coverage by 0.19%.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master     #718      +/-   ##
==========================================
+ Coverage   90.57%   90.76%   +0.19%     
==========================================
  Files         392      392              
  Lines       11013    11013              
  Branches     2080     2080              
==========================================
+ Hits         9975     9996      +21     
+ Misses       1038     1017      -21
@alexandru

This comment has been minimized.

Member

alexandru commented Sep 14, 2018

@jozic just dropping a note that improving the ScalaDocs is the kind of work that really makes a difference. Thanks!

@jozic

This comment has been minimized.

Contributor

jozic commented Sep 14, 2018

thanks @alexandru

i've checked travis logs and it fail because code samples like

    *         (() => ec.execute(() => {
    *           try
    *             cb.onSuccess(thunk)
    *           catch { case NonFatal(e) =>
    *               cb.onError(e)
    *           }
    *         })): Runnable,

don't compile for 2.11 (because of SAM types)
we can add -Xexperimental to make it available on 2.11 or rewrite examples to not use SAM

which way do you guys prefer it?

@alexandru

This comment has been minimized.

Member

alexandru commented Sep 14, 2018

I would change that to a plain Runnable. That cast looks weird anyway.

Or in case ec is a Monix Scheduler, another thing you could do is to use executeAsync, which is an extension method.

@jozic jozic force-pushed the jozic:fix-task-scaladocs branch from fac67b1 to 2cc29d2 Sep 14, 2018

@jozic

This comment has been minimized.

Contributor

jozic commented Sep 14, 2018

jvm builds pass, but js ones don't as samples use jvm only methods, like runSyncUnsafe and Scheduler.io
any ideas how to solve that?

@alexandru

This comment has been minimized.

Member

alexandru commented Sep 14, 2018

Can't we stop this verification for JS?

Alternatively, remove/ rewrite whatever causes issues.

@jozic jozic force-pushed the jozic:fix-task-scaladocs branch from 2cc29d2 to bc5fcbc Sep 15, 2018

@jozic

This comment has been minimized.

Contributor

jozic commented Sep 15, 2018

Can't we stop this verification for JS?

of course we can, silly me :)

just enabled doctestSettings for evalJVM only, looks better now

@jozic

This comment has been minimized.

Contributor

jozic commented Sep 17, 2018

@oleg-py could you please check it out again? thanks

@oleg-py

Looks good. I only found one tiny nit 👍

@@ -154,7 +161,7 @@ import scala.util.{Failure, Success, Try}
*
* {{{
* // Some array of tasks, you come up with something good :-)
* val list: Seq[Task[Int]] = ???
* val list: Seq[Task[Int]] = Seq(Task(10), Task(20), Task(30))
*
* // Split our list in chunks of 30 items per chunk,
* // this being the maximum parallelism allowed

This comment has been minimized.

@oleg-py

oleg-py Sep 17, 2018

Collaborator

Actually, I didn't realize it was splitting the list in the chunk of 30 below... Do Seq.tabulate(100)(Task(_))?

This comment has been minimized.

@jozic

jozic Sep 17, 2018

Contributor

looks good

@jozic jozic force-pushed the jozic:fix-task-scaladocs branch from bc5fcbc to 738b136 Sep 17, 2018

@jozic

This comment has been minimized.

Contributor

jozic commented Sep 19, 2018

how about now? good to go?

@alexandru alexandru merged commit 9bb42f9 into monix:master Sep 19, 2018

1 check passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
@alexandru

This comment has been minimized.

Member

alexandru commented Sep 19, 2018

I merged it because some other changes are coming from myself and I don't want to break this merge.

If you find anything else, open different PRs.

@jozic jozic deleted the jozic:fix-task-scaladocs branch Sep 19, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment