Optics as extension on Companion #825

nomisRev · 2018-04-29T10:25:22Z

Since other generators require companion object I propose the same requirement for optics generation. If so then we can generate optics as extension properties on the companion object instead of global functions. This will improve discoverability and place make for an easier to use API.

val optic = (employeeCompany() compose
  companyAddress() compose
  addressStreet() compose
  streetName())

will become

val optic = (Employee.company compose
  Company.address compose
  Address.street compose
  Street.name)

Thoughts?

…e-discoverability # Conflicts: # modules/core/arrow-annotations-processor/src/main/java/arrow/optics/BoundSetterGenerator.kt # modules/core/arrow-annotations-processor/src/main/java/arrow/optics/OpticsProcessor.kt # modules/docs/arrow-docs/src/main/kotlin/SyntaxHelper.kt

andrzejressel · 2018-04-29T10:37:45Z

This will require declaring empty compation object for annotated classes - I don't like that. Also, this new syntax looks weird to me - It looks like I'm accessing static field.

codecov · 2018-04-29T10:42:33Z

Codecov Report

Merging #825 into master will increase coverage by 0.07%.
The diff coverage is 57%.

@@             Coverage Diff              @@
##             master     #825      +/-   ##
============================================
+ Coverage     44.48%   44.55%   +0.07%     
+ Complexity      633      623      -10     
============================================
  Files           294      298       +4     
  Lines          7466     7492      +26     
  Branches        834      831       -3     
============================================
+ Hits           3321     3338      +17     
- Misses         3844     3851       +7     
- Partials        301      303       +2

Impacted Files	Coverage Δ	Complexity Δ
...ls/src/main/java/arrow/ap/objects/iso/IsoSealed.kt	`0% <ø> (ø)`	`0 <0> (ø)`	⬇️
...rc/main/java/arrow/ap/objects/optional/Optional.kt	`0% <ø> (ø)`	`0 <0> (?)`
...dels/src/main/java/arrow/ap/objects/prism/Prism.kt	`0% <ø> (ø)`	`0 <0> (ø)`	⬇️
...odels/src/main/java/arrow/ap/objects/iso/IsoXXL.kt	`0% <ø> (ø)`	`0 <0> (ø)`	⬇️
...r/models/src/main/java/arrow/ap/objects/iso/Iso.kt	`0% <ø> (ø)`	`0 <0> (?)`
.../src/main/java/arrow/ap/objects/lens/LensSealed.kt	`0% <ø> (ø)`	`0 <0> (ø)`	⬇️
...n/java/arrow/ap/objects/optional/OptionalSealed.kt	`0% <ø> (ø)`	`0 <0> (ø)`	⬇️
...models/src/main/java/arrow/ap/objects/lens/Lens.kt	`0% <ø> (ø)`	`0 <0> (?)`
...va/arrow/ap/objects/prism/PrismWithoutCompanion.kt	`0% <0%> (ø)`	`0 <0> (?)`
...ics/src/main/kotlin/arrow/optics/typeclasses/At.kt	`0% <0%> (ø)`	`0 <0> (ø)`	⬇️
... and 26 more

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 d2cb7a9...eb65be9. Read the comment docs.

nomisRev · 2018-04-29T10:58:19Z

This will require declaring a companion object for annotated classes - I don't like that.

Yes it does and neither do I but we already require it for @instance and @product too. I briefly looked into the compiler plugins to automatically add the companion object but I was in a little over my head on that one. The building that would take quite some time to get familiar with the APIs and required bytecode for companion objects.

Concerning the syntax. I think Person.name is a big win over a global function personName() both for discoverability as user-friendliness. Searching global function by name convention can be really tedious and IDE support is much more friendly when searching companion objects.

That it looks like accessing a static field is fine for me as you could see it as a composition of functional references. @jereksel Any suggestions to improve this?

We could also make them fun instead of val, would that be a better improvement? I wanted to stay away from pre- or post fixes such as Person.nameLens or `Person.

andrzejressel · 2018-04-29T11:28:52Z

So let's do it another way - instead of

val optic = (Employee.company compose
  Company.address compose
  Address.street compose
  Street.name)

Maybe we should go with something like

val optic = (Employee.company compose
  address compose
  street compose
  name)

We could even go a step further

val optic = Employee.company.address.street.name

I'm fine with this additional companion as far as processor will throw readable error when one is missing.

nomisRev · 2018-04-29T11:59:00Z

Snippet 1 & 2 are the same but with a different set of imports.

I thought about snippet 3 but if we do that it would be a DSL on top of snippet 1 but not replace traditional composition. Just like we do with bound setter.

raulraja · 2018-05-14T10:41:49Z

I like the lenses in the companion more than functions on the package but the concern I have here is name collisions.
Employee.name seems to me like a overly generic namespace that resembles a property but not a lens and it's potentially a namespace anyone could claim on its own for any other purpose regarding manipulation of properties in a data class.

nomisRev · 2018-05-14T16:22:05Z

@raulraja I didn't feel the need to add fixes for reasons explained below. I want to avoid confusion so I am open for suggestions.

Employee.name seems to me like a overly generic namespace that resembles a property

That's actually the reason why I choose this option. In Haskell lenses are also referred to as functional references, therefore I thought it made sense to represent them as such. Monocle seems to do something similar for Lens.

This is not only done for lenses but also for optional values. i.e.

@optics data class Person(val name: Name, val age: Age?, val address: Option<Address>)

results in

val Person.Companion.name: Lens<Person, Name> inline get()= ...
val Person.Companion.age: Optional<Person, Age> inline get()= ...
val Person.Companion.address: Optional<Person, Address> inline get()= ...
val Person.Companion.iso: Iso<Person, Tuple3<Name, Age?, Option<Address>>> inline get()= ...

To be complete

sealed class MaybeInt {
  object Nothing: MaybeInt()
  data class Just(val value: Int): MaybeInt()
}

val MaybeInt.Companion.nothing: Prism<MaybeInt, MaybeInt.Nothing> inline get()= ...
val MaybeInt.Companion.just: Prism<MaybeInt, MaybeInt.Nothing> inline get()=...

raulraja · 2018-05-14T17:57:42Z

@nomisRev alright, I'm sold 👍

raulraja · 2018-05-14T17:59:35Z

I also like val optic = Employee.company.address.street.name as @jereksel suggested and if we can do both styles it would be awesome.

nomisRev · 2018-05-15T17:40:39Z

@raulraja I'd really love a DSL like val optic = Employee.company.address.street.name but it needs quite a bit of code gen to allow for it to work.

However I think this is a much stronger feature than the current bounded syntax and am not sure if the additionally generated code is an issue given the power this offers. Also with this style syntax I personnaly don't feel the need anymore to support bounded syntax. Thoughts? @raulraja @jereksel @pakoito @JorgeCastilloPrz

For example to make the following snippet compile

@optics
data class Address(val city: String, val street: Street) {
  companion object
}

@optics
data class Company(val name: String, val address: Address) {
  companion object
}

@optics
data class Employee(val name: String, val company: Company?) {
  companion object
}
val address: Optional<Employee, Address> = Employee.company.address

We need to generate following code

/** For Employee::company **/
inline val <S> Iso<S, Employee>.company: Optional<S, Company> get() = this + Employee.company
inline val <S> Lens<S, Employee>.company: Optional<S, Company> get() = this + Employee.company
inline val <S> Optional<S, Employee>.company: Optional<S, Company> get() = this + Employee.company
inline val <S> Prism<S, Employee>.company: Optional<S, Company> get() = this + Employee.company
inline val <S> Setter<S, Employee>.company: Setter<S, Company> get() = this + Employee.company
inline val <S> Traversal<S, Employee>.company: Traversal<S, Company> get() = this + Employee.company
inline val <S> Fold<S, Employee>.company: Fold<S, Company> get() = this + Employee.company

/** For Company::address **/
inline val <S> Iso<S, Company>.address: Lens<S, Address> get() = this + Company.address
inline val <S> Lens<S, Company>.address: Lens<S, Address> get() = this + Company.address
inline val <S> Optional<S, Company>.address: Optional<S, Address> get() = this + Company.address
inline val <S> Prism<S, Company>.address: Optional<S, Address> get() = this + Company.address
inline val <S> Getter<S, Company>.address: Getter<S, Address> get() = this + Company.address
inline val <S> Setter<S, Company>.address: Setter<S, Address> get() = this + Company.address
inline val <S> Traversal<S, Company>.address: Traversal<S, Address> get() = this + Company.address
inline val <S> Fold<S, Company>.address: Fold<S, Address> get() = this + Company.address

raulraja · 2018-05-15T18:01:47Z

@nomisRev I think that as long as those val's are not initialized until usage it would be fine regardless of the amount of code. If you think the new DSL would supersede the bounded syntax one then great!. We are in a privileged situation now that won't last much longer as we approach 1.0 and whatever makes the barrier of entry to lenses easier the better.

nomisRev · 2018-05-15T18:16:07Z

@raulraja The to-generate-code is quite trivial so I should be able to add that over the weekend.

Since the generated code are all inline val there should be no overhead. I am unsure what happens with inline val if they're not used.

Reason I think the new DSL supersedes the bounded syntax is that we're making actual Optics here instead of bounded instances so they live up to their true potential.

Additionally, I think the bounded syntax is not worth the overhead employee.setter().company.address.street.name.modify(String::toUpperCase) over

Employee.company.address.street.name.modify(employee, String::toUpperCase)

…lity' into simon-optics-improve-discoverability

pakoito · 2018-05-17T23:14:49Z

modules/optics/arrow-optics/src/main/kotlin/arrow/optics/dsl/at.kt

@@ -0,0 +1,84 @@
+package arrow.optics.dsl


How can this file be accessed, if not from the menu?

You mean DSL docs? I updated menu, folder structure, and permalink.

nomisRev · 2018-05-18T06:48:17Z

Re-enabling grained control for DSL will be done in #848

nomisRev · 2018-05-19T16:47:16Z

@arrow-kt/maintainers ready for review :)

pakoito · 2018-05-19T19:49:41Z

...s/core/arrow-annotations-processor/models/src/main/java/arrow/ap/objects/iso/IsoCompanion.kt

+import arrow.optics.optics
+
+@optics([OpticsTarget.ISO])
+data class IsoCompanion(val field: String, val nullable: String?, val option: Option<String>)


Companion object?

Test model to test for a compiler error. https://github.com/arrow-kt/arrow/pull/825/files#diff-8bcf2e9e0361c1d797accab0aede2539R32

raulraja · 2018-05-20T17:36:11Z

modules/core/arrow-annotations-processor/models/src/main/java/arrow/ap/objects/iso/IsoSealed.kt

@@ -6,5 +6,6 @@ import arrow.optics.optics

 @optics([OpticsTarget.ISO])
 sealed class IsoSealed(val field: String, val nullable: String?, val option: Option<String>) {
+  companion object {}


No need for {}

raulraja

Awesome sauce! 👏 👏 This is huge for lenses adoption, This DSL makes it very easy to compose and modify deeply nested structures which is the main feature people want out of an optics lib.

raulraja · 2018-05-20T17:41:11Z

modules/docs/arrow-docs/docs/docs/optics/dsl/README.md

@@ -73,14 +75,14 @@ import arrow.data.*
 val jane = Employee("Jane Doe", Company("Kategory", Address("Functional city", Street(42, "lambda street"))))
 val employees = Employees(listOf(john, jane).k())

-employees.setter().employees.every(ListK.each()).company.address.street.name.modify(String::capitalize)
+Employees.employees.every(ListK.each()).company.address.street.name.modify(employees, String::capitalize)


Is it possible to provide ListK.each() automatically based on the property type internally so the user can just do?

Employees.employees.every().company.address.street.name.modify(employees, String::capitalize)

Also if we detect types for which we now a wrapper exists for example List <-> ListK, is it possible to preserve the user's type List while providing internally a Each instance for its wrapper type?

@raulraja I tried to find an encoding for this but we need true typeclasses AFAIK.

We can do this only for one F due to type erasure and I am unsure if we should favor List/ListK by adding
fun <T, A> Lens<T, ListK<A>>.every(): Traversal<T, A> = this compose ListK.traversal().

Because then we cannot do it for any other F like
fun <T, A> Lens<T, SetK<A>>.every() or fun <T, K, A> Lens<T, MapK<K, A>>.every()

raulraja · 2018-05-20T17:43:07Z

modules/optics/arrow-optics/src/main/kotlin/arrow/optics/syntax/BoundSetter.kt

- * @param S the source of a [BoundSetter]
- * @param A the focus of a [BoundSetter]
- */
-class BoundSetter<S, A>(val value: S, val setter: Setter<S, A>) {


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.

Move Optics std to companion objects (#888) 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.

* Moves generated optics to Companion * Introduces a `.` syntax DSL for composing Optics

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.

nomisRev and others added 11 commits April 18, 2018 18:27

Moved all generated optics to companion objects

b937c16

Update docs Iso

4a6c1df

Update docs Lens

879259d

Update docs Optional

3a0e74b

Update docs Prism

6e55d12

fix AP tests

8773db2

Add OpticsProcessor check to verify unnamed companion object

887e2c8

Add ProductProcessor check to verify companion object

da0e817

Fix bound setter docs and processor

81f0209

Merge branch 'master' into simon-optics-improve-discoverability

e114127

Merge branch 'master' into simon-optics-improve-discoverability

8a36ee4

nomisRev added 3 commits May 1, 2018 16:14

Merge branch 'master' into simon-optics-improve-discoverability

7fbef42

Merge branch 'master' into simon-optics-improve-discoverability

d6a98bf

Merge branch 'master' into simon-optics-improve-discoverability

8a62eb0

nomisRev added 3 commits May 15, 2018 22:46

Add missing Prism compose function on Iso

6e1eec2

Remove DSL code gen

d8d4ec3

Add Optics DSL for Lens

3d0282a

nomisRev and others added 11 commits May 15, 2018 22:49

Remove syntax classes

8076ff1

Merge remote-tracking branch 'origin/simon-optics-improve-discoverabi…

269b4ee

…lity' into simon-optics-improve-discoverability

At Optics DSL support

b45bc2d

Each Optics DSL support

3472e4d

Option.Some Optics DSL support

fc83a60

Fix Lens processor tests

14d670d

Fix Optional processor tests

0cf749f

Fix Prism processors tests

e0b813f

Fix DSL tests

41b804d

Update Optics docs

2ce9535

Merge branch 'master' into simon-optics-improve-discoverability

fb1e81a

nomisRev force-pushed the simon-optics-improve-discoverability branch from 937131b to f345a9a Compare May 17, 2018 20:14

Fix docs

f345a9a

pakoito reviewed May 17, 2018

View reviewed changes

Fix permalink

185cb3d

pakoito reviewed May 19, 2018

View reviewed changes

Rename test models

c9ec7fa

raulraja reviewed May 20, 2018

View reviewed changes

raulraja approved these changes May 20, 2018

View reviewed changes

nomisRev and others added 2 commits May 21, 2018 11:08

Remove empty class body { } from companions

11a2497

Merge branch 'master' into simon-optics-improve-discoverability

eb65be9

nomisRev merged commit e5af8ee into master May 21, 2018

nomisRev deleted the simon-optics-improve-discoverability branch May 21, 2018 15:49

nomisRev mentioned this pull request Jun 13, 2018

Move Optics std to companion objects #888

Merged

nomisRev added a commit that referenced this pull request Jun 26, 2018

Move Optics std to companion objects (#888)

3109e7c

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.

RawToast pushed a commit to RawToast/kategory that referenced this pull request Jul 18, 2018

Optics as extension on Companion (arrow-kt#825)

6b171b0

* Moves generated optics to Companion * Introduces a `.` syntax DSL for composing Optics

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Optics as extension on Companion #825

Optics as extension on Companion #825

nomisRev commented Apr 29, 2018

andrzejressel commented Apr 29, 2018 •

edited

Loading

codecov bot commented Apr 29, 2018 •

edited

Loading

nomisRev commented Apr 29, 2018 •

edited

Loading

andrzejressel commented Apr 29, 2018 •

edited

Loading

nomisRev commented Apr 29, 2018

raulraja commented May 14, 2018

nomisRev commented May 14, 2018

raulraja commented May 14, 2018

raulraja commented May 14, 2018

nomisRev commented May 15, 2018

raulraja commented May 15, 2018

nomisRev commented May 15, 2018 •

edited

Loading

pakoito May 17, 2018

nomisRev May 18, 2018 •

edited

Loading

nomisRev commented May 18, 2018

nomisRev commented May 19, 2018

pakoito May 19, 2018

nomisRev May 19, 2018

raulraja May 20, 2018

raulraja left a comment

raulraja May 20, 2018

nomisRev May 21, 2018

raulraja May 20, 2018

Optics as extension on Companion #825

Optics as extension on Companion #825

Conversation

nomisRev commented Apr 29, 2018

andrzejressel commented Apr 29, 2018 • edited Loading

codecov bot commented Apr 29, 2018 • edited Loading

Codecov Report

nomisRev commented Apr 29, 2018 • edited Loading

andrzejressel commented Apr 29, 2018 • edited Loading

nomisRev commented Apr 29, 2018

raulraja commented May 14, 2018

nomisRev commented May 14, 2018

raulraja commented May 14, 2018

raulraja commented May 14, 2018

nomisRev commented May 15, 2018

raulraja commented May 15, 2018

nomisRev commented May 15, 2018 • edited Loading

pakoito May 17, 2018

Choose a reason for hiding this comment

nomisRev May 18, 2018 • edited Loading

Choose a reason for hiding this comment

nomisRev commented May 18, 2018

nomisRev commented May 19, 2018

pakoito May 19, 2018

Choose a reason for hiding this comment

nomisRev May 19, 2018

Choose a reason for hiding this comment

raulraja May 20, 2018

Choose a reason for hiding this comment

raulraja left a comment

Choose a reason for hiding this comment

raulraja May 20, 2018

Choose a reason for hiding this comment

nomisRev May 21, 2018

Choose a reason for hiding this comment

raulraja May 20, 2018

Choose a reason for hiding this comment

andrzejressel commented Apr 29, 2018 •

edited

Loading

codecov bot commented Apr 29, 2018 •

edited

Loading

nomisRev commented Apr 29, 2018 •

edited

Loading

andrzejressel commented Apr 29, 2018 •

edited

Loading

nomisRev commented May 15, 2018 •

edited

Loading

nomisRev May 18, 2018 •

edited

Loading