Skip to content
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

feat: Add @CurrencyAmount annotation for currency amount validation #1248

Merged

Conversation

bdferris-v2
Copy link
Collaborator

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!

  • Run the unit tests with gradle test to make sure you didn't break anything
  • Format the title like "feat: [new feature short description]". Title must follow the Conventional Commit Specification(https://www.conventionalcommits.org/en/v1.0.0/).
  • Linked all relevant issues

…or currency amounts.

This will be used for various fields in the Fares V2 spec.
@isabelle-dr
Copy link
Contributor

Is this ready for review @bdferris-v2 ? 😊

@bdferris-v2
Copy link
Collaborator Author

Yes.

Copy link
Contributor

@maximearmstrong maximearmstrong left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for this contribution @bdferris-v2! I've added a few comments.

@@ -112,6 +112,11 @@ public boolean process(Set<? extends TypeElement> annotations, RoundEnvironment
new LatLonValidatorGenerator(fileDescriptors).generateValidatorFiles()) {
writeJavaFile(javaFile);
}

for (JavaFile javaFile :
new CurrencyAmountValidatorGenerator().generateValidatorFiles(fileDescriptors)) {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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?

Copy link
Collaborator Author

@bdferris-v2 bdferris-v2 Sep 13, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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.

bdferris-v2 and others added 3 commits September 13, 2022 14:41
…or/CurrencyAmountValidatorGenerator.java

Co-authored-by: Maxime Armstrong <[email protected]>
Copy link
Contributor

@maximearmstrong maximearmstrong left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM! Thanks @bdferris-v2

@maximearmstrong maximearmstrong merged commit 07e8256 into MobilityData:master Sep 14, 2022
@bdferris-v2 bdferris-v2 deleted the issue/1201/fares-currency branch October 7, 2022 18:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants