-
Notifications
You must be signed in to change notification settings - Fork 21
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
Immutable data #49
Immutable data #49
Conversation
Co-authored-by: Jose Gutiérrez de Ory <jose.gutierrez@47deg.com>
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.
LGTM!
Great job @serras!
I've just found a typo.
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.
Couple of nits, and questions. Overall it looks really great 🙌
The "main line" of optics is `Traversal` → `Optional` → `Lens`, which differ | ||
only in the amount of elements they focus on. [`Prism`](../optional-prism) adds a small | ||
twist: it allows not only modifying, but also _creating_ new values and | ||
matching over them. |
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.
Not sure if matching is a term that is as well known in Kotlin as Haskell or Scala. Can we somehow specify it's related to when
?
## Indexed collections | ||
|
||
To exemplify why optionals are useful, let's introduce a few domain classes, | ||
which model a small in-memory database mapping person names to the city in | ||
which they live. | ||
|
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.
Nit: Should we start this document talking about nullable fields? Seems quite a big jump to go immediately to collections 🤔 And IMO we're missing a bit on how this works in the DSL. IIRC we're not generating Optional
anymore and instead only Prism
and using notNull
in the DSL?
IIRC we disabled them from generating (?). I thought we were going to do the same thing for nullable as Option
.
@optics data class Person(val email: String?, val optEmail: Option<String>) {
companion object {
/** Generated */
val email: Lens<Person, String?> = TODO()
val optEmail: Lens<Person, Option<String>> = TODO()
}
}
val x: Optional<Person, String> = Person.email.notNull
val y: Optional<Person, String> = Person.optEmail.some
Just saw this is briefly mentioned at the bottom.
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.
This is mostly based on the fact that I perceived that people had trouble understanding how Optional
works with nullable fields, because of the fact that they cannot change to something which is not null
with lenses. I think that the same idea is better understood for collections (not being able to add or remove elements).
Furthermore, the change to generate Lens
instead of Optional
is due to Arrow 2.x, because it's a breaking change. This only adds a bit more noise to the fact that nullable fields are somehow in flux in Arrow.
In any case, I'm happy to bring back the nullable fields; there's definitely some help from doing it that way (for example, we can compare with ?.
and so on).
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.
This is mostly based on the fact that I perceived that people had trouble understanding how Optional works with nullable fields, because of the fact that they cannot change to something which is not null with lenses. I think that the same idea is better understood for collections (not being able to add or remove elements).
Makes sense, should we include a section on :::danger Nullable types
. Or at least an example and comparing to the collection section at the beginning of the document?
Furthermore, the change to generate Lens instead of Optional is due to Arrow 2.x, because it's a breaking change. This only adds a bit more noise to the fact that nullable fields are somehow in flux in Arrow.
I'm not arguing against this change, I am very happy with the improvements made in Arrow 2.x. Just wondering if we should clarify how it works a bit more in this document.
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.
I'm not arguing against this change, I am very happy with the improvements made in Arrow 2.x. Just wondering if we should clarify how it works a bit more in this document.
No worries, I know that you're fully on board with the new changes. The problem when I was writing it was how to be clear enough about both behaviors, but not confuse the reader with too many small details.
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.
I've added a heading with an explanation of notNull
One important (and sometimes surprising) behavior of optionals is that using | ||
`set` or `modify` only transforms the value if it was already present. That means | ||
that we cannot use `index` to _add_ elements to the database, only to _modify_ | ||
those already present. |
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.
Should we mention here that Lens<Person, String?>
, and ``Lens<Person, Option>` does have this capability?
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.
I've added an additional paragraph explaining this.
Co-authored-by: Simon Vergauwen <nomisRev@users.noreply.github.com>
Co-authored-by: Simon Vergauwen <nomisRev@users.noreply.github.com>
Co-authored-by: Simon Vergauwen <nomisRev@users.noreply.github.com>
Co-authored-by: Simon Vergauwen <nomisRev@users.noreply.github.com>
Co-authored-by: Simon Vergauwen <nomisRev@users.noreply.github.com>
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.
Great stuff, @serras!
Co-authored-by: Francisco Diaz <francisco.d@47deg.com>
Co-authored-by: Francisco Diaz <francisco.d@47deg.com>
If you want to perform a change over the collection, use `modify` over the | ||
lens that corresponds to that field. |
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.
Great addition, this was one of the StackOverflow questions some time ago 🙌
Looks great @serras 🙌 Awesome work 👏 👏 |
No description provided.