-
Notifications
You must be signed in to change notification settings - Fork 451
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
Move Optics std to companion objects #888
Conversation
I see you've created new Api objects for List and such. Rather than having a new one just for this library, it'd be better if we added an object in one of the core or data modules, and add your extfuns from the optics module. That way the object can be extended by all modules, and we centralise all the APIs. |
Codecov Report
@@ Coverage Diff @@
## master #888 +/- ##
=========================================
Coverage ? 45.38%
Complexity ? 652
=========================================
Files ? 302
Lines ? 7745
Branches ? 833
=========================================
Hits ? 3515
Misses ? 3924
Partials ? 306
Continue to review full report at Codecov.
|
…to simon-std-on-companion
@pakoito I made the changes 👍 |
@@ -0,0 +1,3 @@ | |||
package arrow.instances | |||
|
|||
object ListInstances |
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.
Could you please put this file in arrow-data instead? That way we don't need to depend on the instances package when adding other typeclasses to List.
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.
Shouldn't these go to arrow-core
?
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.
It's cheap so why not :D
object SetInstances |
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.
Same as ListInstances! We could also add the others: Map, SortedMap, and SortedSet.
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.
Approved with one suggestion!
0a361f0
to
dd0b002
Compare
Like it was done for generated Optics in arrow-kt#825 this PR moves the Optics of the std to the companion objects of the respective data types.
Like it was done for generated Optics in #825 this PR moves the Optics of the std to the companion objects of the respective data types.