Closed floatdrop closed 1 year ago
I must say I am a bit surprised by this PR. If you choose to reimplement the whole thing why not create a new library and become maintainer?
I must say I am a bit surprised by this PR. If you choose to reimplement the whole thing why not create a new library and become maintainer?
I think Konform has very nice API and there is no need to fracture kotlin ecosystem of validation libraries. At least I would like to try improve Konform as much as I can before that. On the other hand I understand, that this PR has a lot of major changes and could be hard to digest at one go.
Another point to make a PR - I tried to keep API as close to Konform as I can, so it would be awkward to publish another library with similar API as another project.
Would it be ok to talk about these changes, when this PR will be ready to be published?
Turns out it quite hard to pull out all major changes after a couple of month, but I will try anyway.
0.4.0
version constructs Constraints
before validation and grouping them for fields. It can be useful for performance, but blocks eager
implementation (or I don't see how it could be implemented). New implementation runs validation block every time invoke
is called and executes all code, that can change behavior between calls. This also opens door for affects
, that gets value of property, that should affect validation.
For attaching properties and explicit error types Validation
now parameterized with two generic parameters: T
- object under validation and E
– error class. This leads to some boilerplate code for checks (you must specify, what error should check return), but there are ValidationError
interface on which many helpers in errors
package can be implemented and boilerplate should be minimal.
Unfortunately infix required
now must accept two parameters (error and block) – this can not work with infix
, so for now required
looses infix
, but require
helper on validation builder with ValidationError
error type introduced (see readme).
dataPath
was detached from ValidationError
and was converted to own type Path
(from String
) – this can be useful for introspection and traversing objects with reflection after validation returns errors with list of KProperty
to traverse.
invoke
now accept second parameter, which can filter properties under validation. This is useful, when you have objects in DB, that were validated with older version of validation and you need to validate only fields, that were changed by user.
NonEmptyList
Invalid
state of validation returns non-empty list of ErrorWithPath
. I think it is quite useful to mark lists as non-empty by type, but this can be reverted to List
easily.
I've finally spend some time to have a look at the rewrite. It was actually fun to explore and an interesting approach to the problem that Konform is trying to solve. Thanks for taking the time. Especially the change of execution pattern was interesting, as I was trying the same thing a couple of years ago, but ultimately decided to not follow (see below).
I might try to integrate some of the things in this PR into Konform, but the chance of me merging this PR is becoming very small. In fact, the bigger the PR, the less likely it's going to get merged. I am more open to accept individual changes in separate PRs.
With the main problem of this being to big of a PR to merge I can tell you why else I am hesitant to accept this PR.
Konform was originally designed to be declarative. I broke this design by invoking build
at the end of creating a Validation and also by introducing run
. This was done in order to make progress. I was never actually happy with this direction. I would rather prefer Konform to build a validation structure that can be inspected and then build functionality that allows the validation structure to become dynamic once the value is known, by way of an interpreter of the validation structure. I've done a couple of experiments around this so I know it's possible (with some minor possibly non-breaking changes to the API).
If I understand it correctly, the approach in this PR where the builder needs the value in order to build up the validation structure, goes in the exact opposite direction.
This PR is raking up some breaking changes that are convenient for the rewrite, but not from the user-perspective. Existing users would need to migrate and for new users there is going to be a strange mix of styles in the DSL. Like that most functions are extension functions or infix functions, but some like required
and ifPresent
are not. The additional error type parameter that is probably irrelevant for most is another thing.
I'd love to get more focused contributions for Konform, but also I am not opposed to you starting a fork from konform with a similar API. In fact I would see that as a compliment 😊
In addition to the above there are some smaller things. This is more FYI:
As much as possible Konform should stay dependency free. This reduces maintenance and lowers the chance of transitive supply chain attacks. As you already said, the introduction of NonEmptyList
can be reversed and definitely should.
I have seen the outstanding task of reimplementing the deleted test cases, but it is in my opinion not a good idea to do so when the goal is a rewrite.
Think this one is pretty self-explanatory. Many functions currently don't have a single use. Having at least a single simple test would help immensely with building confidence without having to try every new functionality
Thanks for reviewing PR! I wasn’t hoping, that this PR would be merged as is (that’s why I omitted reimplementing test cases - only readme example). All the points you made about konform direction making perfect sense, so feel free to close it.
2 сент. 2022 г., в 15:01, Niklas Lochschmidt @.***> написал(а):
I've finally spend some time to have a look at the rewrite. It was actually fun to explore and an interesting approach to the problem that Konform is trying to solve. Thanks for taking the time. Especially the change of execution pattern was interesting, as I was trying the same thing a couple of years ago, but ultimately decided to not follow (see below).
I might try to integrate some of the things in this PR into Konform, but the chance of me merging this PR is becoming very small. In fact, the bigger the PR, the less likely it's going to get merged. I am more open to accept individual changes in separate PRs.
With the main problem of this being to big of a PR to merge I can tell you why else I am hesitant to accept this PR.
Change to the execution mode
Konform was originally designed to be declarative. I broke this design by invoking build at the end of creating a Validation and also by introducing run. This was done in order to make progress. I was never actually happy with this direction. I would rather prefer Konform to build a validation structure that can be inspected and then build functionality that allows the validation structure to become dynamic once the value is known, by way of an interpreter of the validation structure. I've done a couple of experiments around this so I know it's possible (with some minor possibly non-breaking changes to the API).
If I understand it correctly, the approach in this PR where the builder needs the value in order to build up the validation structure, goes in the exact opposite direction.
Breaking changes to the API
This PR is raking up some breaking changes that are convenient for the rewrite, but not from the user-perspective. Existing users would need to migrate and for new users there is going to be a strange mix of styles in the DSL. Like that most functions are extension functions or infix functions, but some like required and ifPresent are not. The additional error type parameter that is probably irrelevant for most is another thing.
I'd love to get more focused contributions for Konform, but also I am not opposed to you starting a fork from konform with a similar API. In fact I would see that as a compliment 😊
In addition to the above there are some smaller things. This is more FYI:
Introduction of 3rd party libraries
As much as possible Konform should stay dependency free. This reduces maintenance and lowers the chance of transitive supply chain attacks. As you already said, the introduction of NonEmptyList can be reversed and definitely should.
Removal of existing tests
I have seen the outstanding task of reimplementing the deleted test cases, but it is in my opinion not a good idea to do so when the goal is a rewrite.
No tests for new functionality
Think this one is pretty self-explanatory. Many functions currently don't have a single use. Having at least a single simple test would help immensely with building confidence without having to try every new functionality
— Reply to this email directly, view it on GitHub, or unsubscribe. You are receiving this because you authored the thread.
This is PR for solving multiple issues at once (so it will be messy for a white). I will write detailed resume of proposed changes and issues, that should be affected/fixed by this PR. Also API's are not final and are subject for discussion.
For now these issues should be affected by this PR:
54 – Added free standing
ifPresent
andrequired
blocks53 –
*Items
were splitted to separate functions by type52 –
properties
argument added tovalidate
method,affects
helper also introduced to mark dependencies between fields51 – Error type introduced to generic signature
Validation<T, E>
48 – Should be achievable through custom errors type (but requires some additional code)
36 –
required
now acceptsconstructError
argument to provide custom errors (but looses infix)29 –
affects
provides access to property value inside validationBlock
20 –
Pair<Person::name, Person::familyName>> affects { ... }
could be used (test for this needed, I guess)8 –
eager
modifier added to break validation on first errorJSONSchemaStyleConstraintsTest.kt
andValidationBuilderTest.kt
to new implementationErrorWithPath
classIndexed access to errors
back