feat: Add @CurrencyAmount annotation for currency amount validation#1248
Conversation
…or currency amounts. This will be used for various fields in the Fares V2 spec.
|
Is this ready for review @bdferris-v2 ? 😊 |
|
Yes. |
maximearmstrong
left a comment
There was a problem hiding this comment.
Thanks for this contribution @bdferris-v2! I've added a few comments.
core/src/main/java/org/mobilitydata/gtfsvalidator/annotation/CurrencyAmount.java
Outdated
Show resolved
Hide resolved
core/src/main/java/org/mobilitydata/gtfsvalidator/annotation/CurrencyAmount.java
Outdated
Show resolved
Hide resolved
...src/main/java/org/mobilitydata/gtfsvalidator/processor/CurrencyAmountValidatorGenerator.java
Outdated
Show resolved
Hide resolved
| } | ||
|
|
||
| for (JavaFile javaFile : | ||
| new CurrencyAmountValidatorGenerator().generateValidatorFiles(fileDescriptors)) { |
There was a problem hiding this comment.
Is there a reason why this generator would receive the fileDescriptors through the generateValidatorFiles() method instead of the constructor? I'm fine with your approach, I'm just wondering if all generators should follow the same design for consistency and maintainability. Thoughts?
There was a problem hiding this comment.
When I look at all this code in GtfsAnnotationProcessor, there seems to be a recurring pattern of:
for (JavaFile javaFile : new SomeGenerator(fileDescriptors).generateValidatorFiles()) {
writeJavaFile(javaFile);
}
I think a lot of this repeated code could be better simplified with a new interface:
public interface ValidatorGeneratorForFiles {
Iterable<JavaFile> generateValidatorsForFiles(List<GtfsFileDescriptor> fileDescriptors);
}
and then code like:
ImmutableList<ValidatorGeneratorForFiles> generators = ImmutableList.of(new XYZGenerator(), ...);
for (ValidatorGeneratorForFiles generator : generators) {
for (JavaFile javaFile : generator.generateValidatorFiles(fileDescriptors)) {
writeJavaFile(javaFile);
}
}
In that world, it's convenient to construct the generators separately from the file descriptors they'll ultimately be operating on.
Admittedly, that's the theory anyway. Looking at the existing generators, I acknowledge they all currently accept the descriptors via constructors. There's an argument that matches the "command" pattern, where each object encapsulates all the data it needs to executue.
But I couldn't help but look at that code and think it needed a refactor. For example, why do all the generators include their output package as a repeatedly defined static param. Stuff like that should be consolidated as well.
There was a problem hiding this comment.
I agree with you. I think we can keep your code as is in that context. I opened an issue and linked this comment to it.
…urrencyAmount.java Co-authored-by: Maxime Armstrong <[email protected]>
…or/CurrencyAmountValidatorGenerator.java Co-authored-by: Maxime Armstrong <[email protected]>
…urrencyAmount.java Co-authored-by: Maxime Armstrong <[email protected]>
maximearmstrong
left a comment
There was a problem hiding this comment.
LGTM! Thanks @bdferris-v2
This will be used for various fields in the Fares V2 spec to reduce duplicate validation logic. Specifically, I will use this functionality in PR #1228 but I'm splitting it out to simplify the review.
Please make sure these boxes are checked before submitting your pull request - thanks!
gradle testto make sure you didn't break anything