feat: recommended annotation

[KClough]

Summary:

Related to issue #885

Adds a new annotation for recommended fields

Expected behavior:

Provides additional functionality around the recommended annotation. It can now be applied to fields.

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
  - [ ] Include screenshot(s) showing how this pull request works and fixes the issue(s)

@isabelle-dr per your request, this pull request contains just the annotation. I don't think a screenshot makes sense in this scenario.

[CLAassistant]

All committers have signed the CLA.

[maximearmstrong]

Thank you for your contribution @KClough ! This is great work. Overall, if possible, it may be better to split this PR in two so that the notices are added in a second one. Otherwise, only a few comments before approval.

[KClough]

Sorry, it looks like I made a mistake when separating the PRs, I meant for this one to just have the annotation logic. I'll take another pass at it.

[maximearmstrong]

No worries @KClough! I'll take a look at it right after.

[isabelle-dr]

Hi @KClough, thanks for splitting the PRs, this makes it simpler :)

Here is what I understand from the opened PRs:

• PR feat: recommended annotation #1149 (this one) to add a @recommended annotation in the validator
• PR feat: recommended file notice #1151 which closes issue Best Practice: feed_info.txt should be provided (WARNING) #877 to add the missing_required_file notice, which will apply (for now) only to thefeed_info.txt file, but that could be used for other files when the Best Practices evolves.
• PR feat: recommended field annotation #1152 which closes issue Best Practice: feed_info should include start_date, end_date, version, email, url (WARNING) #885 to add the missing_required_field notice, which will apply (for now) only to the feed_info.txt fields mentioned in the issue. Again, we can then use this notice for other fields recommended by the best practices or spec.

If this is accurate, here are some changes to do, in order to make it easier when we look back at these PRs in the future:

1. Can you please update the description of this PR to remove the link to issue feat: frequencies.txt exact_times=0 trips must not have stop_times.timepoint=1 records #887, and update the "Expected behaviour section"?
2. Remove the @recommended in the GTFSFeedInfoSchema file in this PR, to have it only in PR feat: recommended field annotation #1152

[KClough]

Hi @KClough, thanks for splitting the PRs, this makes it simpler :)

Here is what I understand from the opened PRs:

• PR feat: recommended field annotation #1149 (this one) to add a @recommended annotation in the validator
• PR feat: recommended file notice #1151 which closes issue Best Practice: feed_info.txt should be provided (WARNING) #877 to add the missing_required_file notice, which will apply (for now) only to thefeed_info.txt file, but that could be used for other files when the Best Practices evolves.
• PR feat: recommended field annotation #1152 which closes issue Best Practice: feed_info should include start_date, end_date, version, email, url (WARNING) #885 to add the missing_required_field notice, which will apply (for now) only to the feed_info.txt fields mentioned in the issue. Again, we can then use this notice for other fields recommended by the best practices or spec.

If this is accurate, here are some changes to do, in order to make it easier when we look back at these PRs in the future:

1. Can you please update the description of this PR to remove the link to issue feat: frequencies.txt exact_times=0 trips must not have stop_times.timepoint=1 records #887, and update the "Expected behaviour section"?

Resolved above.

2. Remove the @recommended in the GTFSFeedInfoSchema file in this PR, to have it only in PR feat: recommended field annotation #1152

I created a new PR for this: #1156

Sorry for the confusion. 😅

[maximearmstrong]

Thank you for the changes @KClough!

I believe we are missing a few things before we can merge:

• there are two unresolved items above - I believe one of these will fix most of the checks failing in this PR.
• the code format workflow fails. Did you format the code using gradle googleJavaFormat or gradle goJF before pushing your code?
• The branch is out-of-date with the master branch. We will need it to be up-to-date to be able to merge.

[maximearmstrong]

@bdferris-v2, I've added you as a reviewer for this PR. If you get a chance to look at it, your feedback is welcome :)

[maximearmstrong]

2 things that should fix the checks that are failing right now.

[maximearmstrong]

We will need to change the doc string here. I think it makes the Task :aggregateJavadoc to fail here and here.

[maximearmstrong]

My bad, there was an error in my previous suggestion. It makes the Task :aggregateJavadoc to fail here and here.

Suggested change

	* @Recommended
	* {@literal @}Recommended

[bdferris-v2]

I've got a high-level macro comment that may just highlight my ignorance of how this code works, but let me give it a shot.

I feel like there is a model mismatch in how you are treating Required vs Recommended vs Optional in this PR. On one hand, you've got the file and field descriptors, which have separate booleans for isRequired() and isRecommended(). On the other hand, you've introduced a new FieldLevelEnum for passing into RowParser specifying the acceptance level for a given field. This PR, as best I can tell, doesn't actually update the logic in TableLoaderGenerator for setting that FieldLevelEnum when calling RowParser methods, instead relying on the existing isRequired() method and switching between REQUIRED or OPTIONAL, but never actually passing in RECOMMENDED. Maybe that's a function of the way you split up the PR into parts?

So at a tactical level, I think it would be good to update that logic in this PR.

But more generally, it made me wonder how this should actually be modeled overall. So I have questions like:

1. Are all Required fields also Recommended by implication?
2. Should the boolean isRequired() and isRecommended() methods in the descriptors be refactored to use FieldLevelEnum directly instead as a single state?

[maximearmstrong]

@bdferris-v2 Very good point, I hadn't noticed that this logic implementation was missing in TableLoaderGenerator. I also think it should be updated in this PR.

For your questions

1. This is a good question. A field is Required because the specification says it is required. A field is Recommended because the best practices say that the field "should" be there - even if "must" is used in the best practices, we consider it is recommended. Then yes, most probably we can say that Required fields are Recommended by implication. We would not want notices to be created for both though - the highest requirement level reached for a field is the one considered. @isabelle-dr Do you have thoughts on this?

2. Now that you mention it, I think it could be the best. It seems that the required() method in GtfsFieldDescriptor is only used in TableLoaderGenerator to generate the load() and getRequiredColumnNames() methods of the table loaders. Most likely using Enum values would be better. Though, would we keep the isRecommended() and isRequired() methods for the file descriptors as is?

[isabelle-dr]

To answer this first point, I agree with what @maximearmstrong said above. If a file is required, we could say it is also recommended by implication but we don't want to trigger the missing_recommended_file notice for it.

[KClough]

@bdferris-v2 it looks like I messed up this PR when splitting out notices from annotations. I think we're in good spot now.

@maximearmstrong I believe I've resolved all javaDoc issues. The build step passes locally for me.

Once this PR is merged, I'll rebase my other PRs and resolve any other outstanding issues.

[bdferris-v2]

Apologies, but I think my original concern still exists. Namely, you've updated all the methods in RowParser to accept the new FieldLevelEnum, but you haven't updated the call site generated in TableLoaderGenerator to ever pass in a RECOMMENDED value where appropriate.

Specifically, TableLoaderGenerator L331:

"rowParser.$L($L, $T.$L$L)",
    gtfsTypeToParserMethod(field.type()),
    fieldColumnIndex(field.name()),
    RowParser.class,
    field.required() ? "REQUIRED" : "OPTIONAL",  // <= This line right here
    ...

That's the piece I'm still looking for in this PR.

[KClough]

@bdferris-v2 thanks for catching that, I had that line in my notice implementation and missed pulling it over.

I've updated my last commit to include that change.

[isabelle-dr]

Thanks for following-up on this @bdferris-v2!
@maximearmstrong, can you have another look?
I have been following from far, do we need to open an issue to re-iterate the logic for adding the @recommended annotation in a later stage?

[maximearmstrong]

Thank you @KClough and @bdferris-v2! The build passes for me as well. We are ready to merge 🎉

@isabelle-dr I don't think we need to open an issue to re-iterate the logic for adding the @recommended annotation. I think this implementation works well.