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

Clarify @Required annotations for validation #1343

Open
briandonahue opened this issue Feb 28, 2023 · 2 comments
Open

Clarify @Required annotations for validation #1343

briandonahue opened this issue Feb 28, 2023 · 2 comments
Labels
enhancement New feature request or improvement on an existing feature status: Needs discussion We need a discussion on requirements before calling this issue ready

Comments

@briandonahue
Copy link
Collaborator

briandonahue commented Feb 28, 2023

Describe the problem

Currently the @Required annotation is used on the schema files both at a Class level (for required files) and the Property level (for required fields). This can be confusing and recently, we in #1198 we had a scenario where a column was required, but it was not required that the field have data for the column. We discussed adding a new annotation when a column Header is required, but the column data can be empty (not required), which led to discussion of the existing @Required annotation.

Proposed solution

I think for additional clarity, it would make sense to break the original @Required header into two:
@RequiredFile - when the file can not be missing from the feed (valid on the schema Class)
@RequiredValue - when a value is required for the field (
@RequiredColumn - when the column (header) is required, but the field (value) is allowed to be empty

Alternatives you've considered

Leave @Required as-is, and just add the new @ RequiredColumn annotation for the new case.

Additional context

No response

@briandonahue briandonahue added enhancement New feature request or improvement on an existing feature status: Needs triage Applied to all new issues labels Feb 28, 2023
@welcome
Copy link

welcome bot commented Feb 28, 2023

Thanks for opening your first issue in this project! If you haven't already, you can join our slack and join the #gtfs-validators channel to meet our awesome community. Come say hi 👋!

Welcome to the community and thank you for your engagement in open source! 🎉

@briandonahue
Copy link
Collaborator Author

@isabelle-dr mentioned another scenario in #1350 regarding a warning for a RecommendedColumn. I think adding several more specific annotations may be useful.

Currently we have the following:

Current:

Rule Annotation Target Example Type Occurs Notes
File Required @Required Class Agency Error Feed Dual purpose annotation may cause confusion
Column & Value Required @Required Field agencyName Error File & Record Dual purpose annotation may cause confusion
Column & Value Optional @RequiredColumn Field transferType Error File

I propose splitting the @Required annotation into @RequiredFile and @RequiredValue annotations, and adding @RecommendedColumn and potentially a @RecommendedValue annotation would be clearer for readability and testability:

Proposal:

Rule Annotation Target Example Type Occurs Notes
File Required @RequiredFile Class Agency Error Feed
Column Required @RequiredColumn Field transferType Error File Column must exist, values are optional
Value Required @RequiredValue Field agencyName Error Record Column must exist, value is required for each record
Column Recommended @RecommendedColumn Field timepoint Warning File Recommended to include column
Value Recommended @RecommendedValue Field ? Warning Record Recommended to include value

One potential consideration is documenting these in RULES.md could be confusing? E.g. if multiple fields in multiple files are marked with @RequiredValue we would have to illustrate them all in the required_value error section, with links to the correct area of the specification. As opposed to having a custom error notice for each situation which gives the ability to provide a specific example/link for each error.

From a development standpoint I think the proposal is much clearer as long as we feel we can document the errors effectively.

@emmambd emmambd added status: Needs discussion We need a discussion on requirements before calling this issue ready and removed status: Needs triage Applied to all new issues labels Jan 11, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature request or improvement on an existing feature status: Needs discussion We need a discussion on requirements before calling this issue ready
Projects
None yet
Development

No branches or pull requests

2 participants