All committers have signed the CLA.

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.

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

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

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. 😅

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.

@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 :)

@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?

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.

@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.

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.

@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.

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?