feat: Dependency injection with ValidatorProvider#862
feat: Dependency injection with ValidatorProvider#862lionel-nj merged 1 commit intoMobilityData:masterfrom
Conversation
aababilov
requested review from
barbeau and
lionel-nj
and removed request for
lionel-nj
aababilov
force-pushed
the
validator-provider
branch
from
cc023a4 to
c47f978
Compare
lionel-nj
left a comment
lionel-nj
left a comment
There was a problem hiding this comment.
Thanks, a few questions inline. As you'll see, my main interrogation resides in the use of DefaultValidatorProvider.
| * <p>A {@code ValidatorProvider} is a handy pack of validators passed to different parts of the | ||
| * system. Unit tests and users may provide their own implementations of {@code ValidatorProvider}. | ||
| */ | ||
| public interface ValidatorProvider { |
There was a problem hiding this comment.
Should this be an abstract class? So that clients providing their own set of validation rules would not have to provide an implementation for the methods defined here - but I might have misunderstood the usage of this class.
There was a problem hiding this comment.
You see, clients may inherit from DefaultValidatorProvider and override only some methods.
ValidatorProvider as an interface may be fully replaced in a unit tests that does not want to perform any extra validation.
In general, my feeling is that having an interface is cleaner.
There was a problem hiding this comment.
clients may inherit from DefaultValidatorProvider and override only some methods.
This is nice! Also could be used in the future to help support profiles bundled with the canonical validator as well, if I understand correctly. It would be nice to have some documentation in a Markdown file for how this works and example use.
core/src/main/java/org/mobilitydata/gtfsvalidator/validator/TableHeaderValidator.java
Show resolved
Hide resolved
core/src/main/java/org/mobilitydata/gtfsvalidator/validator/ValidatorLoader.java
Outdated
Show resolved
Hide resolved
core/src/test/java/org/mobilitydata/gtfsvalidator/validator/DefaultFieldValidatorTest.java
Outdated
Show resolved
Hide resolved
processor/src/main/java/org/mobilitydata/gtfsvalidator/processor/TableLoaderGenerator.java
Show resolved
Hide resolved
aababilov
force-pushed
the
validator-provider
branch
from
c47f978 to
02a2fb6
Compare
|
Thanks for review! PTAL. |
|
I will add some docs after Lionel merges his #855 |
We pass a single ValidatorProvider object instead of ValidatorContext and ValidatorLoader. This allows us to achieve a list of goals. 1. Better modularity and testability. It is possible to inject a custom ValidatorProvider in the tests. 2. Clients may provide their own validation for phones, emails etc. 3. We open a way to define custom table header validators. 4. We will introduce a concept of critical validators (e.g., foreign key integrity) that will run before all other validators. 5. ValidationContext is now used in a small amount of files, so it will be easier to split it into smaller classes.
aababilov
force-pushed
the
validator-provider
branch
from
02a2fb6 to
81d56a9
Compare
|
I resolved merge conflicts. |
|
How does that look to you guys? That is an important PR to fully switch to the new Validator at Google. |
|
I think that #855 needs more work. Do you mind to merge my PR first? It is a blocker for me. |
|
Thanks, much appreciated! |
3 participants
We pass a single ValidatorProvider object instead of ValidatorContext
and ValidatorLoader. This allows us to achieve a list of goals.
ValidatorProvider in the tests.
integrity) that will run before all other validators.
be easier to split it into smaller classes.