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

Replace ~> with NaturalTransformation #991

Merged
merged 1 commit into from
May 17, 2016

Conversation

markus1189
Copy link
Contributor

After having read NaturalTransformation.scala several times for reference, I found it much easier to read by replacing NaturalTransformation[A,B] with the much better A ~> B.

This pr replaces the occurrences where possible in NaturalTransformation.scala, Free.scala and FreeApplicative.scala.

I am sure there are more places where you could do this, but I first wanted to check if this change is welcome at all ;)

@codecov-io
Copy link

codecov-io commented Apr 23, 2016

Current coverage is 81.99%

Merging #991 into master will decrease coverage by 6.54%

  1. 6 files (not in diff) in ...main/scala/cats/free were deleted. more
  2. 3 files (not in diff) in ...main/scala/cats/free were created. more
  3. 13 files (not in diff) in ...cala/cats/kernel/std were modified. more
    • Misses +111
    • Hits -111
  4. 5 files (not in diff) in ...in/scala/cats/kernel were modified. more
    • Misses +37
    • Hits -37
  5. 2 files (not in diff) in ...ala/cats/kernel/laws were modified. more
    • Misses +25
    • Hits -25
  6. 3 files (not in diff) in ...in/scala/cats/syntax were modified. more
    • Misses +4
    • Hits -5
  7. 7 files (not in diff) in .../main/scala/cats/std were modified. more
    • Misses -11
    • Hits +2
  8. 2 files (not in diff) in ...n/scala/cats/functor were modified. more
  9. 8 files (not in diff) in ...main/scala/cats/data were modified. more
    • Hits -5
  10. 6 files (not in diff) in .../src/main/scala/cats were modified. more
    • Misses +3
    • Hits -6
@@             master       #991   diff @@
==========================================
  Files           214        215     +1   
  Lines          2719       2704    -15   
  Methods        2655       2639    -16   
  Messages          0          0          
  Branches         59         60     +1   
==========================================
- Hits           2407       2217   -190   
- Misses          312        487   +175   
  Partials          0          0          

Sunburst

Powered by Codecov. Last updated by 271c197...a706712

@ceedubs
Copy link
Contributor

ceedubs commented Apr 23, 2016

@markus1189 thanks for bringing this up!

Personally, I kind of prefer ~> to NaturalTransformation as well. However, I think we've kind of settled on using the non-symbolic form of operators and types even when a symbolic form is provided as an alias. This tends to make it less bewildering and easier to google or ask questions about for newcomers. But I can't seem to find anywhere that we have documented this convention in the contributor guide. In fact, it looks like our contributor guide needs a lot of love. I've just created #993 for this.

@markus1189
Copy link
Contributor Author

That's a good point, also currently there already is a mixture of NaturalTransformation and ~>, both in NaturalTransformation and throughout the rest of the code base.

I agree with you that it might be confusing to see ~> without knowing what that is supposed to be. So is the tendency to convert more ~> to NaturalTransformation instead of the other way around? I think at least inside NaturalTransformation having a consistent style would be beneficial (currently e.g. andThen uses NaturalTransformation while or uses ~>. I'd be happy to change the pr tomorrow to translate it the other way around then ;)

@ceedubs
Copy link
Contributor

ceedubs commented Apr 25, 2016

@markus1189 I tend to agree with you that it's best to be consistent. I hate to ask you to fill in the hole that you have just taken the time to dig, but I'd be in favor of changing the ~> references to NaturalTransformation within the code. Maybe it's best if at least one other person chimes in with their opinion before you take the time to make such a change?

I think the docs for FreeMonad use ~> as well. If we are going to use it there, we should probably at least point out that it's an alias for NaturalTransformation, so F ~> G is equivalent to NaturalTransformation[F, G].

@markus1189 markus1189 changed the title Replace NaturalTransformation with the shorter ~> Replace ~> with NaturalTransformation Apr 26, 2016
@markus1189
Copy link
Contributor Author

Since no one else chimed in, I assume that we go for the replacement of symbolic ~> with NaturalTransformation. I think I got all of them and added a short note in the free monad tutorial ;)

`G[_]` (this particular transformation would be written as `F ~> G`).
`G[_]` (this particular transformation would be written as
`NaturalTransformation[F,G]` or as done here using the symbolic
alternative as `F ~> G`).
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Here is the note in the tutorial

@has609
Copy link

has609 commented May 4, 2016

@@ -238,7 +240,7 @@ recursive structure by:
This operation is called Free.foldMap:

-final def foldMap[M[_]](f: S ~> M)(M: Monad[M]): M[A] = ...
+final def foldMap[M[_]](f: NaturalTransformation[S,M])(M: Monad[M]): M[A] = ...

@markus1189
Copy link
Contributor Author

Whoops, missed that one ;)

@ceedubs
Copy link
Contributor

ceedubs commented May 5, 2016

@markus1189 thanks for helping keep us consistent.

I noticed that ~> is still used in a few places in the docs. Is there a reason behind switching some occurrences over to NaturalTransformation but not others?

@markus1189
Copy link
Contributor Author

@ceedubs do you mean the ones in the tutorial for free monads? I think that it doesn't hurt to have ~> there, because it is explicitly mentioned and I think most people outside of cats would prefer ~> anyway, such that it could be a good idea to make readers of the tutorial familiar with the symbolic operator.

Other than that I only found occurrences of ~> inside of tests which I now also replaced. Anything else I am not seeing?

@ceedubs
Copy link
Contributor

ceedubs commented May 7, 2016

@markus1189 I think I was thrown off by the fact that object InMemoryDatasourceInterpreter extends NaturalTransformation[DataOp,Id] shows up in the free tutorial. If we are settling on ~> for the tutorial, then maybe we should change that to object InMemoryDatasourceInterpreter extends (DataOp ~> Id)?

@ceedubs
Copy link
Contributor

ceedubs commented May 7, 2016

Sorry but this PR has merge conflicts.

@markus1189 markus1189 force-pushed the natural-transformation branch 2 times, most recently from 9fba2d5 to 2d99e00 Compare May 8, 2016 17:51
@markus1189
Copy link
Contributor Author

Should be good to go now ;)

@ceedubs
Copy link
Contributor

ceedubs commented May 9, 2016

👍 thanks, @markus1189!

@non
Copy link
Contributor

non commented May 17, 2016

I'm 👍 on this change too. Unfortunately it has conflicts!

@ceedubs ceedubs merged commit defac45 into typelevel:master May 17, 2016
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

Successfully merging this pull request may close these issues.

5 participants