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 all Scaladocs (except Task's) in monix.eval #705

Merged
merged 1 commit into from Sep 5, 2018

Conversation

Projects
None yet
4 participants
@jozic
Contributor

jozic commented Sep 4, 2018

this fixes almost all Scaladocs in monix.eval package

  • Task excluded for now, as it is the biggest source of compile errors, just needs more time
  • TaskApp is also excluded (though the code is compilable now) as it gives this strange warning (promoted to an error under -X-fatal-warnings)
Error:(40, 12) MyApp$2 has a main method with parameter type Array[String], but monix.eval.TaskAppDoctest.MyApp$2 will not be a runnable program.
  Reason: companion contains its own main method, which means no static forwarder can be generated.
    object MyApp extends TaskApp {

I think this is related to this object defined inside test method in generated test, not sure how to fix it

  • also the Scaladoc in TaskLike lead me to removal of execution context parameter from fromFuture

so now instead of

implicit def fromFuture(implicit ec: ExecutionContext): TaskLike[Future]

it's

implicit val fromFuture: TaskLike[Future]

looks like ec wasn't needed, but i'm not sure there wasn't some planned usage for it, lmk about this one

@oleg-py @alexandru please review

@Avasil

Avasil approved these changes Sep 4, 2018

@Avasil

This comment has been minimized.

Collaborator

Avasil commented Sep 4, 2018

looks like ec wasn't needed, but i'm not sure there wasn't some planned usage for it, lmk about this one

That would be correct, it wasn't necessary for deferFuture.

Build fails with:

�[0m[�[0m�[31merror�[0m] �[0m�[0m/home/travis/build/monix/monix/monix-eval/shared/src/main/scala/monix/eval/TaskApp.scala:49:38: Variable name undefined in comment for trait TaskApp in trait TaskApp�[0m
�[0m[�[0m�[31merror�[0m] �[0m�[0m  *           Task(println(s"Hello, ${name}.")).as(ExitCode.Success)�[0m
�[0m[�[0m�[31merror�[0m] �[0m�[0m                                     ^�[0m
�[0m[�[0m�[31merror�[0m] �[0m�[0mone error found�[0m
�[0m[�[0m�[31merror�[0m] �[0m�[0m(Scalaunidoc / �[31mdoc�[0m) Scaladoc generation failed�[0m
�[0m[�[0m�[31merror�[0m] �[0m�[0mTotal time: 46 s, completed Sep 4, 2018 4:16:43 AM�[0m
@alexandru

This comment has been minimized.

Member

alexandru commented Sep 4, 2018

Ah, my mistake. Yes, ExecutionContext isn't needed there, we'll have to fix it.

@alexandru

This comment has been minimized.

Member

alexandru commented Sep 4, 2018

@Avasil btw that error is from the $ character having special meaning in ScalaDoc. It has to be escaped, I don't remember how. Maybe by doubling it.

@@ -79,7 +79,7 @@ object TaskLike extends TaskLikeImplicits0 {
/**
* Converts to `Task` from [[scala.concurrent.Future]].
*/
implicit def fromFuture(implicit ec: ExecutionContext): TaskLike[Future] =
implicit val fromFuture: TaskLike[Future] =

This comment has been minimized.

@Avasil

Avasil Sep 4, 2018

Collaborator

Could you also fix Observable?

This comment has been minimized.

@jozic

jozic Sep 4, 2018

Contributor

fixed in ObservableLike

@jozic jozic force-pushed the jozic:all-but-task branch from cb32710 to cd8f212 Sep 4, 2018

fix all Scaladocs (except Task's) in monix.eval + remove s.c.Executio…
…nContext from `fromFuture` in TaskLike and ObservableLike

@jozic jozic force-pushed the jozic:all-but-task branch from cd8f212 to 57347d6 Sep 4, 2018

@codecov

This comment has been minimized.

codecov bot commented Sep 4, 2018

Codecov Report

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

@@            Coverage Diff             @@
##           master     #705      +/-   ##
==========================================
+ Coverage   90.58%   90.61%   +0.03%     
==========================================
  Files         391      391              
  Lines       10991    10991              
  Branches     2052     2052              
==========================================
+ Hits         9956     9960       +4     
+ Misses       1035     1031       -4
@jozic

This comment has been minimized.

Contributor

jozic commented Sep 5, 2018

@alexandru can we merge this? or some changes required?

@oleg-py

oleg-py approved these changes Sep 5, 2018

This is awesome, and I even have nothing to complain about. Great solid work, right there 👍

@@ -535,9 +536,9 @@ object TaskCircuitBreaker {
* when the `Open` state is to transition to [[HalfOpen]].
*
* It is calculated as:
* {{{
* ```scala

This comment has been minimized.

@alexandru

alexandru Sep 5, 2018

Member

Does this Github-flavored markdown annotation work?

This comment has been minimized.

@jozic

jozic Sep 5, 2018

Contributor

sbt-doctest ignores it, if that's what you ask
i did that as the code refers to fields, and is not compilable as is

To me this particular scaladoc example is not really an example, but implementation explanation
to make it compilable we will have to write smth like

  val open = Open(1234, 5 seconds)
  require(open.startedAt + open.resetTimeout.toMillis == open.expiresAt)

but not sure if it's helpful or any better
wdyt?

This comment has been minimized.

@alexandru

alexandru Sep 5, 2018

Member

It’s indeed not a sample. But we have to have ScalaDoc compile it without producing garbage and I don’t think this does.

One thing we can do is to do that inline via single back-ticks.

This comment has been minimized.

@jozic

jozic Sep 5, 2018

Contributor

you mean

|`startedAt + resetTimeout.toMillis`

instead of 

|```scala
|startedAt + resetTimeout.toMillis
|```

?

This comment has been minimized.

@alexandru

alexandru Sep 5, 2018

Member

Yes, that can work.

This comment has been minimized.

@jozic

jozic Sep 5, 2018

Contributor

ok, will address that in PR for Task

@alexandru

This comment has been minimized.

Member

alexandru commented Sep 5, 2018

@jozic we can merge it of course. Thanks a lot.

@alexandru alexandru merged commit 98bf3e6 into monix:master Sep 5, 2018

1 check passed

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

@jozic jozic deleted the jozic:all-but-task branch Sep 5, 2018

@jozic

This comment has been minimized.

Contributor

jozic commented Sep 5, 2018

cool, thanks, will focus on fixing Task's docs, that is a big one :)

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