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 recommended field notice #1157

Merged
merged 4 commits into from
Jun 29, 2022

Conversation

KClough
Copy link
Collaborator

@KClough KClough commented May 18, 2022

Summary:

Related #885

Expected behavior:

Adds the recommended field notice.

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)

@KClough KClough changed the title **Summary:** feat: apply recommended field-annotation-to-feed-info May 18, 2022
@github-actions
Copy link
Contributor

This contribution does not follow the conventions set by the Google Java style guide. Please run the following command line at the root of the project to fix formatting errors: ./gradlew goJF.

@KClough KClough changed the title feat: apply recommended field-annotation-to-feed-info feat: add recommended field notice May 18, 2022
@github-actions
Copy link
Contributor

Thank you for this contribution! 🍰✨🦄

Information about source corruption

0 out of 1248 sources are corrupted.

Acceptance test details

The changes in this pull request did not trigger any new errors on known GTFS datasets from the MobilityDatabase.
Download the full acceptance test report for commit 5632971 here (report will disappear after 90 days).

@KClough KClough force-pushed the feat/recommended-field-notice branch from 440a6ae to cbef9f8 Compare May 20, 2022 04:09
@github-actions
Copy link
Contributor

This contribution does not follow the conventions set by the Google Java style guide. Please run the following command line at the root of the project to fix formatting errors: ./gradlew goJF.

@github-actions
Copy link
Contributor

Thank you for this contribution! 🍰✨🦄

Information about source corruption

0 out of 1248 sources are corrupted.

Acceptance test details

The changes in this pull request did not trigger any new errors on known GTFS datasets from the MobilityDatabase.
Download the full acceptance test report for commit 0af3983 here (report will disappear after 90 days).

@KClough KClough marked this pull request as draft June 24, 2022 00:12
@KClough KClough force-pushed the feat/recommended-field-notice branch from cbef9f8 to 3b2cef9 Compare June 24, 2022 01:18
@KClough KClough requested a review from maximearmstrong June 24, 2022 01:48
@KClough KClough marked this pull request as ready for review June 24, 2022 01:48
@KClough KClough marked this pull request as draft June 24, 2022 01:52
@KClough KClough force-pushed the feat/recommended-field-notice branch 2 times, most recently from 980541c to 4ee2769 Compare June 24, 2022 02:05
@KClough KClough marked this pull request as ready for review June 24, 2022 02:10
@github-actions
Copy link
Contributor

Thank you for this contribution! 🍰✨🦄

Information about source corruption

0 out of 1248 sources are corrupted.

Acceptance test details

The changes in this pull request did not trigger any new errors on known GTFS datasets from the MobilityDatabase.
Download the full acceptance test report for commit 8adedf5 here (report will disappear after 90 days).

@github-actions
Copy link
Contributor

Thank you for this contribution! 🍰✨🦄

Information about source corruption

0 out of 1248 sources are corrupted.

Acceptance test details

The changes in this pull request did not trigger any new errors on known GTFS datasets from the MobilityDatabase.
Download the full acceptance test report for commit b1eab8f here (report will disappear after 90 days).

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 @KClough! A few things before approval.

@@ -0,0 +1,35 @@
/*
* Copyright 2020 Google LLC
Copy link
Contributor

Choose a reason for hiding this comment

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

Same as in #1151: you can update the copyright if you'd like.

*
* <p>Severity: {@code SeverityLevel.ERROR}
*/
public class MissingRecommendedFieldNotice extends ValidationNotice {
Copy link
Contributor

Choose a reason for hiding this comment

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

We would need the changes to RULES.md to be done in this PR if possible.

@@ -112,6 +113,14 @@ public String asString(int columnIndex, FieldLevelEnum level) {
noticeContainer.addValidationNotice(
new MissingRequiredFieldNotice(
fileName, row.getRowNumber(), header.getColumnName(columnIndex)));
} else if (level == FieldLevelEnum.RECOMMENDED && s == null) {
Copy link
Contributor

Choose a reason for hiding this comment

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

Also, I think we would need unit tests in RowParserTest.java to test the behavior of asString() depending on FieldLevelEnum value.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good call. I've added some unit tests for both Recommended and Required. Let me know if you'd like me to cover any additional cases.

@KClough KClough force-pushed the feat/recommended-field-notice branch from 82edfbb to 3ad06de Compare June 29, 2022 19:42
@KClough KClough marked this pull request as draft June 29, 2022 19:43
@github-actions
Copy link
Contributor

Thank you for this contribution! 🍰✨🦄

Information about source corruption

0 out of 1248 sources are corrupted.

Acceptance test details

The changes in this pull request did not trigger any new errors on known GTFS datasets from the MobilityDatabase.
Download the full acceptance test report for commit 5974c59 here (report will disappear after 90 days).

KClough and others added 3 commits June 29, 2022 16:45
…arser.java

Co-authored-by: Maxime Armstrong <46797220+maximearmstrong@users.noreply.github.com>
@KClough KClough force-pushed the feat/recommended-field-notice branch from c7156af to 2330794 Compare June 29, 2022 20:46
@github-actions
Copy link
Contributor

This contribution does not follow the conventions set by the Google Java style guide. Please run the following command line at the root of the project to fix formatting errors: ./gradlew goJF.

@KClough KClough marked this pull request as ready for review June 29, 2022 20:47
@KClough KClough force-pushed the feat/recommended-field-notice branch from 2330794 to 507e50d Compare June 29, 2022 20:49
@github-actions
Copy link
Contributor

Thank you for this contribution! 🍰✨🦄

Information about source corruption

0 out of 1248 sources are corrupted.

Acceptance test details

The changes in this pull request did not trigger any new errors on known GTFS datasets from the MobilityDatabase.
Download the full acceptance test report for commit 9fd785c here (report will disappear after 90 days).

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, thank you @KClough!

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.

2 participants