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

Add missing UnorderedTraverse syntax #2148

Merged
merged 1 commit into from
Jan 25, 2018
Merged

Add missing UnorderedTraverse syntax #2148

merged 1 commit into from
Jan 25, 2018

Conversation

andyscott
Copy link
Contributor

@andyscott andyscott commented Jan 11, 2018 •
edited
Loading

Addresses #2147:

The import cats.syntax.traverse._ doesn't provide the .unorderedTraverse. This is inconsistent, as the import cats.syntax.foldable._ does provide the unorderedFold syntax.

@andyscott andyscott changed the title Add missing .unorderedTraverse syntax Add missing UnorderedTraverse syntax Jan 11, 2018
@andyscott
Copy link
Contributor Author

2.10 and 2.11 JVM failed the binary compatibility check. /cc @kailuowang

@kailuowang
Copy link
Contributor

You can't touch an existing trait, you have to have the traverse object extend this extra trait instead of through the TraverseSyntax trait

@andyscott
Copy link
Contributor Author

Aah yes. I'll get that switched today.

@codecov-io
Copy link

codecov-io commented Jan 11, 2018 •
edited
Loading

Codecov Report

Merging #2148 into master will increase coverage by 0.01%.
The diff coverage is n/a.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2148      +/-   ##
==========================================
+ Coverage   94.64%   94.66%   +0.01%     
==========================================
  Files         328      328              
  Lines        5548     5548              
  Branches      213      213              
==========================================
+ Hits         5251     5252       +1     
+ Misses        297      296       -1
Impacted Files Coverage Δ
testkit/src/main/scala/cats/tests/CatsSuite.scala 70% <ø> (ø) ⬆️
core/src/main/scala/cats/UnorderedTraverse.scala 100% <0%> (+50%) ⬆️

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 a296b8e...6375e01. Read the comment docs.

kailuowang
kailuowang reviewed Jan 11, 2018
View reviewed changes
core/src/main/scala/cats/syntax/traverse.scala Outdated
@@ -2,3 +2,4 @@ package cats
package syntax

trait TraverseSyntax extends Traverse.ToTraverseOps
trait TraverseSyntaxBinCompat0 extends UnorderedTraverse.ToUnorderedTraverseOps
Copy link
Contributor

Choose a reason for hiding this comment

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

shall we just name this one UnorderedTraverseSyntax since we dont' have one?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We could. Is there any merit to keeping it as similar to Foldable/UnorderedFoldable (which doesn't have UnorderedFoldableSyntax)?

Copy link
Contributor

@kailuowang kailuowang Jan 12, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

Foldable/UnorderedFoldable is the unconventional one. I am referring to
trait FoldableSyntax extends Foldable.ToFoldableOps with UnorderedFoldable.ToUnorderedFoldableOps
The convention in Cats is that the syntax trait of a TC does NOT extend the syntax of its parent TC. For users to get the parent syntax, they have to either explicitly import the parent syntax, or using the all inclusive import.
So I think having an UnorderdTraverseSyntax trait is more conventional.

TBC, I am okay with cats.syntax.traverse the object extending the UnorderedTraverseSyntax, but it's not really conventional in Cats.
More conventional way would be adding a cats.syntax.unorderedTraverse and having it extending the UnorderedTraverseSyntax, along with making cats.syntax.all and cats.implicits to extends this trait as well as a work-around for keeping bin-compat.

Copy link
Contributor

Choose a reason for hiding this comment

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

sorry about the bike shedding here. The main reason I am discussing more about it is that this would set up a precedence for working around bin compat breakage. although it's a bit atypical

@kailuowang kailuowang added the bug label Jan 11, 2018
@kailuowang kailuowang added this to the 1.1 milestone Jan 11, 2018
@kailuowang
Copy link
Contributor

also if this this not available in cats.implicits._ we need to have that one extend this trait as well.

@kailuowang
Copy link
Contributor

@andyscott how do you want to proceed with this PR? I'll be happy to make the rename changes for you if you want.

@andyscott
Copy link
Contributor Author

andyscott commented Jan 22, 2018 via email

LukaJCB
LukaJCB previously approved these changes Jan 24, 2018
View reviewed changes
Copy link
Member

@LukaJCB LukaJCB left a comment

Choose a reason for hiding this comment

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

Thanks a bunch, Andy! :)

kailuowang
kailuowang reviewed Jan 24, 2018
View reviewed changes
core/src/main/scala/cats/syntax/all.scala
@@ -48,3 +48,6 @@ trait AllSyntax
with ValidatedSyntax
with VectorSyntax
with WriterSyntax

trait AllSyntaxBinCompat0
Copy link
Contributor

@kailuowang kailuowang Jan 24, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

One more small thing, if we make this one abstract class (and maybe even have it extends AllSyntax) we can add more traits to it going forward.

Copy link
Contributor Author

@andyscott andyscott Jan 24, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

How about an abstract class AllSyntaxBinCompat extends AllSyntax with AllSyntaxBinCompat0?

Copy link
Contributor

@kailuowang kailuowang Jan 24, 2018 •
edited
Loading

Choose a reason for hiding this comment

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

then what's the purpose of AllSyntaxBinCompat0? just as a versioning scheme?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

A downstream consumer can't mix in the abstract class but they could mix in AllSyntax and AllSyntaxBinCompat0 to get roughly the same effect. Without AllSyntaxBinCompat0, but then you'd have to remember that AllSyntax is missing UnorderedTraverse syntax. So it's versioning scheme to facilitate downstream use.

I'm not sure if it's worth it though. It's just an idea.

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. Makes sense to me

@andyscott andyscott force-pushed the unorderedTraverse-syntax branch 2 times, most recently from 671c93f to 6375e01 Compare January 25, 2018 03:36
@kailuowang kailuowang merged commit dc6b643 into typelevel:master Jan 25, 2018
This was referenced Jan 25, 2018
Open
Merged
@peterneyens peterneyens mentioned this pull request Mar 25, 2018
Closed
@andyscott andyscott deleted the unorderedTraverse-syntax branch July 13, 2018 22:08
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@kailuowang kailuowang kailuowang approved these changes

@LukaJCB LukaJCB LukaJCB left review comments

Assignees
No one assigned
Labels
bug
Projects
None yet
Milestone
1.1
Development

Successfully merging this pull request may close these issues.
4 participants
@andyscott @kailuowang @codecov-io @LukaJCB