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

Cleaning up docs to no longer use fine grained imports #2664

Merged

Conversation

calvinbrown085
Copy link
Contributor

#2651
@LukaJCB @tpolecat

How does this look to you guys? Hopefully I got them all, happy to take another sweep through if you know of any I missed.

Copy link
Member

@LukaJCB LukaJCB left a comment

Choose a reason for hiding this comment

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

Looks good to me, thank you very much! :)

Copy link
Contributor

@kailuowang kailuowang left a comment

Choose a reason for hiding this comment

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

Thanks!

@kailuowang kailuowang merged commit b781a1a into typelevel:master Dec 6, 2018
@@ -212,8 +212,7 @@ This function is provided by Cats via the `Traverse[List]` instance and syntax,
tutorial.

```tut:book:silent
import cats.instances.list._
import cats.syntax.traverse._
import cats.implicits

Choose a reason for hiding this comment

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

Did this miss the ._?

Copy link
Contributor

Choose a reason for hiding this comment

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

oops, missed that, I will fix that in a separate PR.

Copy link
Member

Choose a reason for hiding this comment

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

Why didn't this broke the build?

Copy link
Contributor

Choose a reason for hiding this comment

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

the import isn't really needed for the code since it's already imported at previous blocks. I guess unused import isn't checked in tut.

Copy link
Member

Choose a reason for hiding this comment

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

If it is not needed I'd remove it or require that each paragraph needs to specify its imports again with maybe tut's :reset modifier.

Copy link
Contributor

@kailuowang kailuowang Dec 6, 2018

Choose a reason for hiding this comment

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

I read the page again and I can see all these imports are confusing. It looks like it would be cumbersome to reset every paragraph.

I suggested that we remove import cats.implicits._ from them and add it to the top of the page, with something like:

First, bring cats instances into the scope by

import cats.implicits._

@kailuowang kailuowang added this to the 1.6 milestone Jan 8, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants