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

Make Scaladoc example compile and modernize it #2083

Merged
merged 1 commit into from
Jun 26, 2021

Conversation

igstan
Copy link
Contributor

@igstan igstan commented Jun 26, 2021

I'm just reading through the source code of the library so I'm fixing such issues as I bump into them. In this case:

  • release takes the allocated resource as a param
  • Use IO.println
  • Use Cats Effect's Random

@vasilmkd
Copy link
Member

I like this. Should we also maybe add the required imports at the beginning of the code block? That way the examples can immediately be copy/pasted.

@igstan
Copy link
Contributor Author

igstan commented Jun 26, 2021

@vasilmkd I've thought about that too, but I wasn't sure. Since you're thinking the same, then I'll add them. It certainly causes no issues.

@vasilmkd
Copy link
Member

@igstan Check the output in the GitHub actions, you can see some small things that you've missed that you can fix. 😄

@igstan
Copy link
Contributor Author

igstan commented Jun 26, 2021

@vasilmkd oops, yes 😅 Fixing now.

@vasilmkd
Copy link
Member

No worries, I'm pretty sure you can run doc in sbt to run this check, before pushing (for a faster turnaround).

@igstan
Copy link
Contributor Author

igstan commented Jun 26, 2021

I've amended the commit with a fix for the CI issue, plus the extra imports needed to run the example. This time I ran +doc locally, thanks :D

  * `release` takes the allocated resource as a param
  * Use IO.println
  * Use Cats Effect's `Random`
@vasilmkd
Copy link
Member

Looks great.

@rossabaker rossabaker merged commit 60dfea5 into typelevel:series/3.x Jun 26, 2021
@igstan
Copy link
Contributor Author

igstan commented Jun 26, 2021

Thank you, both!

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