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.

Sign up for GitHub

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

Jump to bottom

Fix easy typos #3002

Merged
merged 5 commits into from
Aug 26, 2019
Merged

Fix easy typos #3002

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
5 commits
Select commit Hold shift + click to select a range
befb456
Fix instance name
travisbrown Aug 22, 2019
3b6e98c
Fix ZipNonEmptyX Eq instance names
travisbrown Aug 22, 2019
c3798cf
Fix guidelines, which manage to be inconsistent in adjacent lines :)
travisbrown Aug 22, 2019
b5148af
Merge branch 'master' into topic/fix-easy-typos
travisbrown Aug 23, 2019
694d0bb
Fix deprecation arguments
travisbrown Aug 23, 2019
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 4 additions & 1 deletion core/src/main/scala/cats/data/NonEmptyList.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -500,7 +500,10 @@ object NonEmptyList extends NonEmptyListInstances {
ZipNonEmptyList(fa.value.zipWith(fb.value) { case (a, b) => (a, b) })
}

implicit def zipNelEq[A: Eq]: Eq[ZipNonEmptyList[A]] = Eq.by(_.value)
@deprecated("Use catsDataEqForZipNonEmptyList", "2.0.0-RC2")
private[data] def zipNelEq[A: Eq]: Eq[ZipNonEmptyList[A]] = catsDataEqForZipNonEmptyList[A]

implicit def catsDataEqForZipNonEmptyList[A: Eq]: Eq[ZipNonEmptyList[A]] = Eq.by(_.value)
}
}

Expand Down
5 changes: 4 additions & 1 deletion core/src/main/scala/cats/data/NonEmptyVector.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -428,6 +428,9 @@ object NonEmptyVector extends NonEmptyVectorInstances with Serializable {
ZipNonEmptyVector(fa.value.zipWith(fb.value) { case (a, b) => (a, b) })
}

implicit def zipNevEq[A: Eq]: Eq[ZipNonEmptyVector[A]] = Eq.by(_.value)
@deprecated("Use catsDataEqForZipNonEmptyVector", "2.0.0-RC2")
private[data] def zipNevEq[A: Eq]: Eq[ZipNonEmptyVector[A]] = catsDataEqForZipNonEmptyVector[A]

implicit def catsDataEqForZipNonEmptyVector[A: Eq]: Eq[ZipNonEmptyVector[A]] = Eq.by(_.value)
}
}
6 changes: 5 additions & 1 deletion core/src/main/scala/cats/data/Op.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -20,8 +20,12 @@ sealed abstract private[data] class OpInstances extends OpInstances0 {
implicit def catsDataCategoryForOp[Arr[_, _]](implicit ArrC: Category[Arr]): Category[Op[Arr, *, *]] =
new OpCategory[Arr] { def Arr: Category[Arr] = ArrC }

implicit def catsKernelEqForOp[Arr[_, _], A, B](implicit ArrEq: Eq[Arr[B, A]]): Eq[Op[Arr, A, B]] =
implicit def catsDataEqForOp[Arr[_, _], A, B](implicit ArrEq: Eq[Arr[B, A]]): Eq[Op[Arr, A, B]] =
new OpEq[Arr, A, B] { def Arr: Eq[Arr[B, A]] = ArrEq }

@deprecated("Use catsDataEqForOp", "2.0.0-RC2")
private[data] def catsKernelEqForOp[Arr[_, _], A, B](implicit ArrEq: Eq[Arr[B, A]]): Eq[Op[Arr, A, B]] =
catsDataEqForOp[Arr, A, B]
}

sealed abstract private[data] class OpInstances0 {
Expand Down
2 changes: 1 addition & 1 deletion docs/src/main/tut/guidelines.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -80,7 +80,7 @@ therefore name our implicits according to the following rules:
- 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, an implicit instance of `Monoid` for `List` defined in the package `Kernel` should be named `catsKernelMonoidForList`.
As an example, an implicit instance of `Monoid` for `List` defined in the package `Kernel` should be named `catsKernelStdMonoidForList`.
Copy link
Contributor

Choose a reason for hiding this comment

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

I see you are trying to make the example consistent with the last rule. But now I think about it, I'd suggest that we remove the last rule because:
1, it's not enforced in kernel anyway due to historical reasons.
2, it's kind of unintuitive. It's not easy for contributors to just look at existing names and figure this rule out. Yes, I know the current inconsistent names are not easier to figure out a rule. But in case we will be able to fix all the names in the future, at which point this rule will stand out as unintuitive.

Not sure if we should spend much time bike shedding these rules, the names themselves provide no value whatsoever, the long names actually make the code harder to read (the type signature provides all the information). We just need them to be unique enough.

Copy link
Contributor Author

@travisbrown travisbrown Aug 25, 2019 •
edited
Loading

Choose a reason for hiding this comment

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

I don't care at all about the specifics of the rules here, just that the guidelines are followed consistently, and that they are internally consistent. All of the instances in cats-kernel follow the rule you're suggesting we remove (including catsKernelStdMonoidForList itself), so if we remove the rule we're just asking for more inconsistency.

At this point I don't really care about any of this—I was just trying to take care of some low-hanging-fruit clean-up before 2.0 for an issue that bothers me and a few other people. If it's a big deal we can shelve it.


This rule is relatively flexible. Use what you see appropriate. The goal is to maintain uniqueness and avoid conflicts.

Expand Down
2 changes: 1 addition & 1 deletion tests/src/test/scala/cats/tests/OpSuite.scala
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -10,7 +10,7 @@ import cats.kernel.laws.discipline.EqTests

class OpSuite extends CatsSuite {
{
implicit val catsKernelEqForOp = Op.catsKernelEqForOp[Function1, Int, MiniInt]
implicit val catsDataEqForOp = Op.catsDataEqForOp[Function1, Int, MiniInt]
checkAll("Op[Function1, Int, MiniInt]", EqTests[Op[Function1, Int, MiniInt]].eqv)
checkAll("Eq[Op[Function1, Int, MiniInt]]", SerializableTests.serializable(Eq[Op[Function1, Int, MiniInt]]))
}
Expand Down