-
-
Notifications
You must be signed in to change notification settings - Fork 1.2k
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
Add .nested
syntax.
#2262
Conversation
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 |
Yes, I can add a test tomorrow. |
@adelbertc I added a test. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
thanks!
@@ -38,6 +38,7 @@ trait AllSyntax | |||
with MonadErrorSyntax | |||
with MonadSyntax | |||
with MonoidSyntax | |||
with NestedSyntax |
There was a problem hiding this comment.
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
I changed |
@@ -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 { |
There was a problem hiding this comment.
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
There was a problem hiding this comment.
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
.
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Done.
@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 Report
@@ 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
Continue to review full report at Codecov.
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍 Thanks!
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 thatis 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.