Skip to content
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

Updated tutorial to new doc site + CE 3.0.0-RC2 #1799

Merged
merged 2 commits into from
Mar 21, 2021
Merged

Updated tutorial to new doc site + CE 3.0.0-RC2 #1799

merged 2 commits into from
Mar 21, 2021

Conversation

lrodero
Copy link
Contributor

@lrodero lrodero commented Mar 21, 2021

Updated tutorial for CE 3.0.0-RC2. Changes to note:

  • The producer/consumer section does not use semaphores any longer. Instead it is coded using the same approach than in the implementation of CE3's std/Queue.scala.
  • Some links had to be removed as they are not present (yet?) in the current version of the new website.
  • Scattered syntax fixes added.
  • Current build.sbt in the text points to 3.0.0-RC2. As soon as final version is released I can prepare a small change so it points to 3.0.0.

This new version of the tutorial can be read here.

Code samples used in the tutorial are available here.

Copy link
Member

@djspiewak djspiewak left a comment

Choose a reason for hiding this comment

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

Absolutely amazing! Thank you for tackling this! It was on my todo list, but my todo list is massive at this point…

Quick question: can we adjust the code snippets to build with mdoc so that they don't go stale?

Comment on lines +375 to +380
guard <- Semaphore[IO](1)
count <- inputOutputStreams(origin, destination, guard).use { case (in, out) =>
guard.permit.use { _ =>
transfer(in, out)
}
}
Copy link
Member

Choose a reason for hiding this comment

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

Would it be more clear to rewrite this in the following fashion?

  val rsrc = for {
    guard <- Resource.eval(Semaphore[IO](1))
    _ <- inputOutputStreams(origin, destination, guard)
    _ <- guard.permit
    count <- Resource.eval(transfer(in, out))
  } yield count

  rsrc.use(IO.pure(_))

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Well... it's undeniably more elegant, but I'm having trouble myself reading the code (sorry :) ), I guess other newcomers can find it also a bit daunting. Also, it doesn't compile... I think I get the idea but I'd need to refactor the code to make it work.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If you don't mind I'd rather let the code as it's now. I still think it will be easier for people with little or no experience with CE. Also, the change would require some substantial rewrite I think :/ .

docs/tutorial.md Outdated
orig = new File(args(0))
dest = new File(args(1))
count <- copy(orig, dest)
_ <- Console[IO].println(s"$count bytes copied from ${orig.getPath} to ${dest.getPath}")
Copy link
Member

Choose a reason for hiding this comment

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

IO.println also works. :-)

Copy link
Contributor Author

@lrodero lrodero Mar 21, 2021

Choose a reason for hiding this comment

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

Yes! But there are Console[IO].errorln calls in the snippets while there is no IO.errorln. So for the sake of homogeneity I wasn't sure what to do here. I'll change it and explain that IO.printlnis just Console[IO].println underneath.

Copy link
Member

Choose a reason for hiding this comment

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

Totally fair!

docs/tutorial.md Outdated
Comment on lines 469 to 472
WARNING: To properly test cancelation, You should also ensure that
`fork := true` is set in the sbt configuration, otherwise sbt will
intercept the cancelation because it will be running the program
in the same JVM as itself.
Copy link
Member

Choose a reason for hiding this comment

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

This warning should not be required any longer since IOApp detects this scenario and does the Right Thing™

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Cool! Removing the warning then 👍

docs/tutorial.md Outdated
Comment on lines 664 to 665
_ <- if(iO.exists(_ % 10000 == 0)) Console[F].println(s"Consumed ${iO.get} items") else Sync[F].unit
} yield ()) >> consumer(queueR)
Copy link
Member

Choose a reason for hiding this comment

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

Should we just recommend better-monadic-for so we can pull the consumer(queueR) into the for-comprehension?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

In fact I put the recursive call outside the for so it's easily spotted by the reader. Like, 'this is the main thing to do and then we start all over again'. But if you think it would be actually clearer putting the recursive call inside the for I'd be happy to change it.

Copy link
Member

Choose a reason for hiding this comment

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

It's kind of a stylistic thing. IMO I prefer having it inside, the same way it would be if you were writing something like this in an imperative style.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

No prob then, changing it 👍

@lrodero
Copy link
Contributor Author

lrodero commented Mar 21, 2021

Glad to help :) , sorry I didn't undertake this sooner.

No prob on using mdoc, but I have never used it before. Any modifier I should use? compile-only maybe?

@djspiewak
Copy link
Member

djspiewak commented Mar 21, 2021

No prob on using mdoc, but I have never used it before. Any modifier I should use? compile-only maybe?

I think so? My knowledge of mdoc rounds to zero. :-D @keynmol might have some suggestions

@keynmol
Copy link

keynmol commented Mar 21, 2021

@lrodero given that the docs/ folder should'be already covered by mdoc, then yeah, changing all your snippets (apart from SBT ones) from

```scala

to

```scala mdoc:compile-only

Will make sure they're compiled along with the code, but they won't be executed (which for bare IO means nothing without unsafeRunSync or using a special modifier which I totally was going to finish but never did.

Couple of points:

  1. Mdoc processes snippets linearly, so if you see any conflicts (is already defined bla bla), just add :nest to the list of modifiers
  2. Ideally running the snippets would be cool, but for a tutorial it's actually more beneficial to see the self contained IOApp snippet, rather than just the individual bits of IO actions running

@keynmol
Copy link

keynmol commented Mar 21, 2021

And your magic SBT watch command for gradually updating the tutorial is:

sbt:root> docs/mdoc --in docs/tutorial.md --watch

(mdoc has its own watch mode for inputs)

@lrodero
Copy link
Contributor Author

lrodero commented Mar 21, 2021

Thanks for the tips @keynmol ! Ok I think I got it. Two things to note:

  1. I've addressed most of @djspiewak comments, save the first one as it requires a refactor that can lead to a rabbit hole of text rewrites 😅
  2. mdoc works fine. But I'm not using it in a couple of snippets as anyway they are not supposed to compile.

@djspiewak djspiewak merged commit 8510dab into typelevel:series/3.x Mar 21, 2021
@lrodero lrodero deleted the update-tutorial-3.x branch March 21, 2021 22:50
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