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

Document naming implicits according to @non s comment in #1061 #1920

Merged
merged 3 commits into from
Sep 23, 2017

Conversation

tbje
Copy link
Contributor

@tbje tbje commented Sep 20, 2017

- Implicits should start with "cats" -- a third-party will less likely do this.
- The type and the type class should be mentioned in the name.

As an example, for `Monoid[List[A]]`, the name of the implicit should be `catsMonoidForList`.
Copy link
Contributor

Choose a reason for hiding this comment

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

Thanks so much! We actually also include the package name in the name now.
catsPackageNameTypeclassNameForDataStruct

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thank you for letting me know. Would the following be correct ?

In a widely-used library it's important to minimize the chance that the names of implicits will be used by others and therefore name our implicits according to the following rules:

  • Implicits should start with "cats" followed by the package name.
  • The type and the type class should be mentioned in the name.

As an example, for cats.kernel.Monoid[List[A]], the name of the implicit should be catsKernelMonoidForList.

Copy link
Collaborator

Choose a reason for hiding this comment

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

We should probably mention that in cats.instances we don't follow this rule and use catsStd.....

Copy link
Contributor

Choose a reason for hiding this comment

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

The package used in the name is package where the instance is defined, your example gives the impression that it's the package where the type class is defined.
BTW, we are not really strictly following this rule, for instance, in cats.instances we named an instances as
catsStdInstancesForOption
https://github.com/typelevel/cats/blob/master/core/src/main/scala/cats/instances/option.scala#L8

here Std is to represent the package instances which used to be called std, we didn't change the name here because catsInstancesInstancesForOption would be weird.
the next word Instances in the name is used here because this instance is an instance for multiple type classes.
So we do have some flexibility here.

Copy link
Contributor

Choose a reason for hiding this comment

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

My recommendation would be just

In a widely-used library it's important to minimize the chance that the names of implicits will be used by others and therefore name our implicits according to the following rules:

* Implicits should start with "cats" followed by the package (where instance is defined) name.
* The type and the type class should be mentioned in the name.
* If the instance is for multiple type classes, use `InstancesFor` instead 
* This rule is relatively flexible. Use what you see appropriate. The goal is to maintain the uniqueness and avoid conflicts. 

Copy link
Contributor Author

Choose a reason for hiding this comment

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

It seems like Std is used when the instance is for a Scala standard library type.

Copy link
Contributor

@edmundnoble edmundnoble Sep 21, 2017

Choose a reason for hiding this comment

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

I've gone through the instances with @tbje and as far as I can tell the current revision is correct in all cases.

@codecov-io
Copy link

codecov-io commented Sep 20, 2017

Codecov Report

Merging #1920 into master will increase coverage by <.01%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #1920      +/-   ##
==========================================
+ Coverage   95.54%   95.55%   +<.01%     
==========================================
  Files         248      248              
  Lines        4426     4430       +4     
  Branches      116      117       +1     
==========================================
+ Hits         4229     4233       +4     
  Misses        197      197
Impacted Files Coverage Δ
...ree/src/main/scala/cats/free/FreeApplicative.scala 100% <0%> (ø) ⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 15a5240...a7ad647. Read the comment docs.

- If the instance is for multiple type classes, use `InstancesFor` instead of a type class name.
- If the instance is for a standard library type add `Std` after the package. i.e. `catsStdShowForVector` and `catsKernelStdGroupForTuple`.

As an example, for `cats.kernel.Monoid[List[A]]`, the name of the implicit should be `catsKernelMonoidForList`.
Copy link
Contributor

Choose a reason for hiding this comment

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

This example may give the false impression that the package name is the package of the type class. How about

As an example, an implicit instance of `Monoid` for `List` defined in the package `Kernel` should be named as `catsKernelMonoidForList`

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 very much @tbje !

Copy link
Contributor

@ceedubs ceedubs left a comment

Choose a reason for hiding this comment

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

Thanks @tbje!

@ceedubs ceedubs merged commit 38063e8 into typelevel:master Sep 23, 2017
@tbje
Copy link
Contributor Author

tbje commented Sep 24, 2017

Thank you for guiding me !

@kailuowang kailuowang added this to the 1.0.0-RC1 milestone Oct 13, 2017
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.

6 participants