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

Add .nested syntax. #2262

Merged
merged 4 commits into from
May 22, 2018
Merged

Add .nested syntax. #2262

merged 4 commits into from
May 22, 2018

Conversation

danielkarch
Copy link
Contributor

I attended @iravid's talk at the typelevel summit and was surprised that I hadn't known about Nested yet.
Also, I found the syntax a bit clunky. I would propose to add a .nested extension method so that

listOfOption.nested

is equivalent to

Nested(listOfOption)

Since many people import cats.implicits._ this change would also make it easier to discover .nested through an IDE's syntax completion.

@adelbertc
Copy link
Contributor

LGTM, can you also add a simple syntax test for this? https://github.com/typelevel/cats/blob/master/tests/src/test/scala/cats/tests/SyntaxSuite.scala

@danielkarch
Copy link
Contributor Author

Yes, I can add a test tomorrow.

@danielkarch
Copy link
Contributor Author

@adelbertc I added a test.

@iravid iravid mentioned this pull request May 21, 2018
kailuowang
kailuowang previously approved these changes May 21, 2018
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!

@kailuowang kailuowang dismissed their stale review May 21, 2018 14:17

missed bin compat

@@ -38,6 +38,7 @@ trait AllSyntax
with MonadErrorSyntax
with MonadSyntax
with MonoidSyntax
with NestedSyntax
Copy link
Contributor

@kailuowang kailuowang May 21, 2018

Choose a reason for hiding this comment

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

unfortunately changing AllSyntax break binary compatibility on scala 2.11-. As a workaround, please let the AllSyntaxBinCompat1 above extend the new NestedSyntax

@danielkarch
Copy link
Contributor Author

I changed AllSyntaxBinCompat1 to extend NestedSyntax. I am not an experienced library author, so I read up on it here. However, I don't yet understand why this change is supposed to fix binary compatibility issues. Do you have a any reading material that explains it?

@@ -24,7 +25,7 @@ import cats.syntax.AllSyntax
*
* None of these tests should ever run, or do any runtime checks.
*/
object SyntaxSuite extends AllInstances with AllSyntax {
object SyntaxSuite extends AllInstances with AllSyntax with NestedSyntax {
Copy link
Contributor

Choose a reason for hiding this comment

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

Is this needed? It should be part of AllSyntax already right

Copy link
Contributor Author

Choose a reason for hiding this comment

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

After @kailuowang's comment I removed it from AllSyntax and added it to AllSyntaxBinCompat1. So I need to add either with NestedSyntax here or with AllSyntaxBinCompat1.

Copy link
Contributor

Choose a reason for hiding this comment

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

Probably AllSyntaxBinCompat1 to avoid the need to add other extensions in the future, one and done.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Done.

@kailuowang
Copy link
Contributor

@danielkarch it could be confusing if you don't know the history. But mainly the difference between the two traits is that AllSyntax is old and already released while AllSyntaxBinCompat1 is not.

@codecov-io
Copy link

Codecov Report

Merging #2262 into master will decrease coverage by 0.28%.
The diff coverage is 100%.

Impacted file tree graph

@@            Coverage Diff             @@
##           master    #2262      +/-   ##
==========================================
- Coverage   94.96%   94.67%   -0.29%     
==========================================
  Files         333      334       +1     
  Lines        5799     5784      -15     
  Branches      218      210       -8     
==========================================
- Hits         5507     5476      -31     
- Misses        292      308      +16
Impacted Files Coverage Δ
core/src/main/scala/cats/syntax/nested.scala 100% <100%> (ø)
testkit/src/main/scala/cats/tests/CatsSuite.scala 33.33% <0%> (-36.67%) ⬇️
core/src/main/scala/cats/syntax/either.scala 85% <0%> (-14.17%) ⬇️
core/src/main/scala/cats/data/IorT.scala 97.7% <0%> (-0.06%) ⬇️
core/src/main/scala/cats/Eval.scala 98.7% <0%> (-0.05%) ⬇️
core/src/main/scala/cats/data/Ior.scala 98.47% <0%> (-0.02%) ⬇️
laws/src/main/scala/cats/laws/MonadLaws.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 88e25ee...150da99. Read the comment docs.

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!

@kailuowang kailuowang merged commit 6353e99 into typelevel:master May 22, 2018
@danielkarch danielkarch deleted the add-nested-syntax branch May 22, 2018 10:52
@kailuowang kailuowang added this to the 1.2 milestone May 30, 2018
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