Consistency for ops classes #1456

edmundnoble · 2016-11-06T21:27:28Z

Extend AnyVal when possible, unless there are only macro ops
1a. Don't take implicit params in the ops class param list, again unless
there are macro ops, or they can't extend AnyVal.
(there is one exception to this and a comment explaining it; it's EqSyntax)
Make all conversions to ops classes final. They are all declared in
traits, so users with their own syntax hierarchy have no chance of
inlining unless they are marked final. Plus, it's consistent.
Make all ops class constructor fields private by default, to avoid
polluting the namespace of the type.
Edit: 3 is not possible for Scala 2.10, so it was removed from this PR.

codecov-io · 2016-11-06T22:27:31Z

Codecov Report

Merging #1456 into master will not change coverage.
The diff coverage is 91.66%.

@@           Coverage Diff           @@
##           master    #1456   +/-   ##
=======================================
  Coverage   93.08%   93.08%           
=======================================
  Files         250      250           
  Lines        3989     3989           
  Branches      135      136    +1     
=======================================
  Hits         3713     3713           
  Misses        276      276

Impacted Files	Coverage Δ
core/src/main/scala/cats/syntax/semigroupk.scala	`0% <ø> (ø)`	⬆️
core/src/main/scala/cats/syntax/semigroup.scala	`0% <ø> (ø)`	⬆️
core/src/main/scala/cats/syntax/bitraverse.scala	`50% <ø> (ø)`	⬆️
core/src/main/scala/cats/syntax/monadFilter.scala	`0% <ø> (ø)`	⬆️
.../src/main/scala/cats/syntax/applicativeError.scala	`100% <ø> (ø)`	⬆️
...ore/src/main/scala/cats/syntax/contravariant.scala	`0% <ø> (ø)`	⬆️
core/src/main/scala/cats/syntax/order.scala	`0% <ø> (ø)`	⬆️
core/src/main/scala/cats/syntax/monadError.scala	`100% <ø> (ø)`	⬆️
...re/src/main/scala/cats/syntax/traverseFilter.scala	`0% <ø> (ø)`	⬆️
core/src/main/scala/cats/syntax/apply.scala	`100% <ø> (ø)`	⬆️
... and 26 more

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 9c3130e...22d15ea. Read the comment docs.

johnynek · 2016-11-06T22:30:47Z

Can we add these style rules to a doc somewhere? Ideally, we could tool something like wart-remover to check them.

johnynek · 2016-11-06T22:35:29Z

In spirit, I like this, but I do hope sooner or later we get serious about preserving some binary compatibility. This has a large binary incompatibility cross-section.

kailuowang · 2016-11-07T01:45:19Z

@edmundnoble thanks very much for this! Sorry about raising the val in Ops class issue which probably wasted quite some time of yours. I suspected that 2.10 might be the cause for not making those private at the first place but didn't get the chance to verify.
I agree there is value in documenting these style rules including issue that 2.10 preventing the self val to be private.

edmundnoble · 2016-11-07T03:14:57Z

I added a lil something in this PR for the ops classes guidelines, while I was fixing the merge conflict. If it's better somewhere else I can take it right back out.

kailuowang · 2016-11-07T16:15:05Z

LGTM, 👍 Thanks again

johnynek · 2016-11-07T17:13:42Z

I added breaking change to this since it breaks binary compatibility (not source in most cases). I think that is appropriate.

ceedubs · 2017-01-06T16:08:37Z

Given that we are merging other binary-incompatible changes, I think that I give this a 👍. Notably this is different than Simulacrum, which doesn't use value classes (if I recall correctly). @mpilquist do you think that value classes are appropriate in these cases?

@johnynek are you okay with us merging the binary-incompatible change? I've been MIA for a while, so to be honest I don't really know what current plans are on Cats releases/compatibility.

edmundnoble · 2017-01-06T18:27:19Z

Rebased.

johnynek · 2017-01-07T00:41:30Z

Two things:

binary compatibility is not binary (pun intended). The more incompatible changes we make, the greater the chance we cause an issue for a user.
we should investigate using abstract class much more than trait, since it is much friendlier for binary compatibility. For linear hierarchies which we often use for priorities, we could probably use more abstract classes.

johnynek

looks good. Noted a couple of untested methods this trips over (and thus the diff has lower coverage than our current average).

johnynek · 2017-01-07T00:42:50Z

core/src/main/scala/cats/syntax/cartesian.scala


-  def <*[B](fb: F[B])(implicit F: Functor[F]): F[A] = F.map(typeClassInstance.product(self, fb)) { case (a, b) => a }
+  final def <*[B](fb: F[B])(implicit F: Functor[F]): F[A] = F.map(typeClassInstance.product(self, fb)) { case (a, b) => a }


codecov says this method is untested.

johnynek · 2017-01-07T00:44:27Z

core/src/main/scala/cats/syntax/monoid.scala

-final class MonoidOps[A: Monoid](lhs: A) {
-  def isEmpty(implicit eq: Eq[A]): Boolean = Monoid[A].isEmpty(lhs)(eq)
+final class MonoidOps[A](val lhs: A) extends AnyVal {
+  def isEmpty(implicit A: Monoid[A], eq: Eq[A]): Boolean = A.isEmpty(lhs)(eq)


this method is untested.

kailuowang · 2017-04-11T21:47:51Z

@edmundnoble would you like to resolve conflicts and merge (it has two sign-offs)?

edmundnoble · 2017-04-12T17:44:17Z

Gotcha. Will do today.

1. Extend AnyVal when possible, unless there are macro ops 1a. Don't take implicit params in the ops class param list, again unless there are macro ops. 2. Make all conversions to ops classes final. They are all declared in traits, so users with their own syntax hierarchy have no chance of inlining unless they are marked final. Plus, it's consistent. 3. Make all ops class constructor fields private by default, to avoid polluting the namespace of the type. Remove private val part, because scala 2.10 doesn't support it Ops class guidelines Add CartesianTest

edmundnoble mentioned this pull request Nov 6, 2016

Either .left, .right syntax #1454

Merged

edmundnoble force-pushed the consistent-ops-classes branch from 5bee74f to 06a1dce Compare November 6, 2016 21:38

edmundnoble force-pushed the consistent-ops-classes branch 2 times, most recently from 957317b to 874c331 Compare November 7, 2016 03:13

edmundnoble added the Binary Breaking label Nov 7, 2016

edmundnoble force-pushed the consistent-ops-classes branch from 874c331 to 5e57e35 Compare January 6, 2017 18:26

johnynek approved these changes Jan 7, 2017

View reviewed changes

edmundnoble force-pushed the consistent-ops-classes branch from 5e57e35 to 5296069 Compare January 7, 2017 23:45

kailuowang approved these changes Apr 11, 2017

View reviewed changes

kailuowang added this to the 1.0.0-MF milestone Apr 11, 2017

edmundnoble force-pushed the consistent-ops-classes branch from 5296069 to 22d15ea Compare April 13, 2017 04:44

edmundnoble merged commit 19c3a37 into typelevel:master Apr 13, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Consistency for ops classes #1456

Consistency for ops classes #1456

edmundnoble commented Nov 6, 2016 •

edited

Loading

codecov-io commented Nov 6, 2016 •

edited

Loading

johnynek commented Nov 6, 2016

johnynek commented Nov 6, 2016

kailuowang commented Nov 7, 2016 •

edited

Loading

edmundnoble commented Nov 7, 2016

kailuowang commented Nov 7, 2016

johnynek commented Nov 7, 2016

ceedubs commented Jan 6, 2017

edmundnoble commented Jan 6, 2017

johnynek commented Jan 7, 2017

johnynek left a comment

johnynek Jan 7, 2017

johnynek Jan 7, 2017

kailuowang commented Apr 11, 2017

edmundnoble commented Apr 12, 2017


		def <*[B](fb: F[B])(implicit F: Functor[F]): F[A] = F.map(typeClassInstance.product(self, fb)) { case (a, b) => a }
		final def <*[B](fb: F[B])(implicit F: Functor[F]): F[A] = F.map(typeClassInstance.product(self, fb)) { case (a, b) => a }

Consistency for ops classes #1456

Consistency for ops classes #1456

Conversation

edmundnoble commented Nov 6, 2016 • edited Loading

codecov-io commented Nov 6, 2016 • edited Loading

Codecov Report

johnynek commented Nov 6, 2016

johnynek commented Nov 6, 2016

kailuowang commented Nov 7, 2016 • edited Loading

edmundnoble commented Nov 7, 2016

kailuowang commented Nov 7, 2016

johnynek commented Nov 7, 2016

ceedubs commented Jan 6, 2017

edmundnoble commented Jan 6, 2017

johnynek commented Jan 7, 2017

johnynek left a comment

Choose a reason for hiding this comment

johnynek Jan 7, 2017

Choose a reason for hiding this comment

johnynek Jan 7, 2017

Choose a reason for hiding this comment

kailuowang commented Apr 11, 2017

edmundnoble commented Apr 12, 2017

edmundnoble commented Nov 6, 2016 •

edited

Loading

codecov-io commented Nov 6, 2016 •

edited

Loading

kailuowang commented Nov 7, 2016 •

edited

Loading