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

Uniquify implicit names #1061

Closed
non opened this issue May 24, 2016 · 11 comments
Closed

Uniquify implicit names #1061

non opened this issue May 24, 2016 · 11 comments

Comments

@non
Copy link
Contributor

non commented May 24, 2016

Paul Phillips mentions that in a widely-used library it's important to minimize the chance that the names of its implicits will be used by others. To that end I suggest that we review our implicits and rename them according to the following rules:

  1. Make sure that the implicits start with cats -- it's less likely that a third-party will do this.
  2. Make sure that both the type and the type class are mentioned in the name.

So for example, for Monoid[List[A]] the name should be something like catsMonoidForList or similar.

@non
Copy link
Contributor Author

non commented May 24, 2016

Since a single across-the-board PR is likely to conflict with every other open PR, we should either accomplish this piecemeal, or coordinate ahead of time. If you want to work on this issue please comment here so we can avoid duplicating work and plan for a minimum amount of conflict.

@ceedubs
Copy link
Contributor

ceedubs commented May 24, 2016

As an example of how name collisions can be problematic, in this gitter conversation, an upgrade from cats 0.5 to 0.6 broke someone's code because of a collision on the name listMonoid (they had created one for java.util.List).

@dwijnand
Copy link
Contributor

Note that not only implicits that provide typeclass instances, but also syntax/enrich-my-type implicits suffer from the same exact problem.

A banal example is having two different implicit classes from different libraries called AnyOps.

@ghost
Copy link

ghost commented May 25, 2016

Would this also be an issue for simulacrum generated implicits?
For example, for Invariant, we get implicit def toInvariantOps. Should that be implicit def toCatsFunctorInvariantOps?

N.B. Note the inclusion of the full package names, should this be standard as well?

@milessabin
Copy link
Member

Would this also be an issue for simulacrum generated implicits?

Potentially yes.

Bear in mind that this can't be fixed by using freshName in a macro, because the names, even if they're never typed by a human, are public definitions and have to be stable between builds. There's some code in export-hook to deal with this.

@kailuowang
Copy link
Contributor

I can work on this one. How about just tackle the std first?

kailuowang added a commit to kailuowang/cats that referenced this issue May 27, 2016
kailuowang added a commit to kailuowang/cats that referenced this issue May 27, 2016
kailuowang added a commit to kailuowang/cats that referenced this issue May 27, 2016
non added a commit that referenced this issue May 30, 2016
renamed implicit defs in data,  part 2 of #1061
@ceedubs
Copy link
Contributor

ceedubs commented Jun 3, 2016

@kailuowang do you think that this is done after #1066 and #1068? Or should we cover the other modules too? Collisions outside of the std package seem unlikely to me since people don't have to import the instances explicitly, but I guess there is something to be said about consistency.

@kailuowang
Copy link
Contributor

I was thinking about doing syntax after the first two parts merged. I am out of the country now but i should be able to resume the work a week or so later.

@ceedubs
Copy link
Contributor

ceedubs commented Jun 4, 2016

@kailuowang ah, I hadn't thought about syntax - that sounds like an important one. Thank you!

kailuowang added a commit to kailuowang/cats that referenced this issue Jun 10, 2016
ceedubs added a commit that referenced this issue Jun 12, 2016
uniquify implicit names in syntax part 3 of #1061
@kailuowang
Copy link
Contributor

It seems to me that most implicits names are uniquified after the 3 mentioned PRs.

ceedubs added a commit to ceedubs/cats that referenced this issue Jun 14, 2016
I think this is the last of the implicit names for typelevel#1061.
@ceedubs
Copy link
Contributor

ceedubs commented Jun 18, 2016

I think this is all wrapped up now.

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

No branches or pull requests

5 participants