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

Narrow Imports from Cats in Core package #2539

Merged
merged 1 commit into from May 21, 2019

Conversation

@diesalbla
Copy link
Contributor

commented May 1, 2019

  • We remove every wild-card import from the cats, cats.data, or cats.effect packages.
  • We try to narrow down imports of cats.implicits._ to one or two specific imports from cats.syntax or cats.instances.

@diesalbla diesalbla force-pushed the diesalbla:narrow_cats_imports branch 2 times, most recently from 6f6f6e3 to b2996e9 May 1, 2019

@rossabaker

This comment has been minimized.

Copy link
Member

commented May 5, 2019

Our contributors' guide speaks a little about imports, but not to these particular issues. We enforce scalafmt, but scalafmt doesn't have a lot to say about imports, which leads to this being one of the more variant areas of style. We could tighten up the recommendations.

  1. I am mildly 👍 on explicit imports from cats, cats.data, etc. Especially as tooling has gotten better to make this easier.

  2. I am more 👎 on a la carte imports for instances and syntax. The names still aren't explicit. Nor should they be, because they have intentionally long names to avoid accidental shadowing. Finding the right objects is a barrier for new users and a chore for experienced users. And despite claims of faster compilation, I've never seen a signficant difference in practice.

@diesalbla

This comment has been minimized.

Copy link
Contributor Author

commented May 6, 2019

Finding the right objects is a barrier for new users and a chore for experienced users.

@rossabaker I can personally understand the desire to use cats.implicits._ import. I have worked with some Scala applications where compilation times were over a minute long, and I can relate that it is frustrating to lose a minute because of an inaccurate cats import.

However, since http4s is not an application but a library, I would say that here the best criteria is to reduce the number of cats imports, which is to say, to constrain the capabilities each part of the code needs.

The code in the PR is already looking for a compromise: at some point, where there are four or five syntax or instances imports, a syntax.all._ or an .instances.all._ is used.

In any case, these changes are doing no harm.

@rossabaker

This comment has been minimized.

Copy link
Member

commented May 15, 2019

My experience is that these imports don't substantially affect compile time. (Imports in other libraries that derive instances, for sure. These imports, no.) I'd want to see a very striking difference on this project to change my mind. Maintainer time is far more precious than CPU time, and I find narrowing implicit imports to be a chore. And mixing a la carte imports with regular imports has come up a couple times on Gitter recently, and I really don't want to encourage a style that raises the barrier to new contributors, given how uninformative the compilation failures are.

I still like eliminating wild cards on things named in the code.

@diesalbla

This comment has been minimized.

Copy link
Contributor Author

commented May 15, 2019

@rossabaker Ok. I will apply the corrections later today.

@diesalbla diesalbla force-pushed the diesalbla:narrow_cats_imports branch 3 times, most recently from 7c04569 to 4ce9ad5 May 15, 2019

@aeons

aeons approved these changes May 20, 2019

@rossabaker

This comment has been minimized.

Copy link
Member

commented May 20, 2019

👍 after conflict resolution

@diesalbla diesalbla force-pushed the diesalbla:narrow_cats_imports branch from 7b98920 to c314ad5 May 20, 2019

Narrow Imports from Cats, Cats-Effect, and FS2.
We remove wild-card imports from the `cats`, `cats.data`, or
`cats.effect` packages, and try to narrow them down.

@diesalbla diesalbla force-pushed the diesalbla:narrow_cats_imports branch from c314ad5 to d30fae1 May 20, 2019

@aeons aeons merged commit 2dba53f into http4s:master May 21, 2019

1 check passed

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

@diesalbla diesalbla deleted the diesalbla:narrow_cats_imports branch May 22, 2019

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants
You can’t perform that action at this time.