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 a rule that stations (location_type 1) must be the parent_station of some stop (location_type 0). #1493

Merged
merged 8 commits into from
Jun 21, 2023

Conversation

bradyhunsaker
Copy link
Contributor

Summary:

Part of #153.

The new notice is of type INFO because there is nothing in the specification or best practices stating that stations should have stops. There is a discussion about changing this in #1348.

As part of the change, this renames ParentLocationTypeValidator to the more general name ParentStationValidator.

For unit testing it is now necessary to consider two types of notices, so all "contains()" calls are changed to "containsExactly()" or "containsExactlyElementsIn()" to be sure that additional notices are not ignored.

Expected behavior:

If a location of type 1 (station) never appears as the parent station of a location of type 0 (stop/platform), then issue a notice.

Please make sure these boxes are checked before submitting your pull request - thanks!

  • [ X] Run the unit tests with gradle test to make sure you didn't break anything
  • [ X] Format the title like "feat: [new feature short description]". Title must follow the Conventional Commit Specification(https://www.conventionalcommits.org/en/v1.0.0/).
  • [X ] Linked all relevant issues
  • Include screenshot(s) showing how this pull request works and fixes the issue(s)

This renames ParentLocationTypeValidator to the more general name ParentStationValidator.

For unit testing it is now necessary to consider two types of notices, so all "contains()" calls are changed to "containsExactly()" or "containsExactlyElementsIn" to be sure that additional notices are not ignored.
Copy link
Member

@davidgamez davidgamez left a comment

Choose a reason for hiding this comment

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

Hi @bradyhunsaker, few changes have been merged since your first PR regarding the notice documentation generation that impacts your contribution. I added a few comments to align this PR with the latest changes before approval.

@bradyhunsaker
Copy link
Contributor Author

Thanks, David! I accepted your suggested edit and removed the super() call. Any other comments?

@davidgamez
Copy link
Member

Thanks, David! I accepted your suggested edit and removed the super() call. Any other comments?

Thanks, @bradyhunsaker. One more ask, Can you review the formatting(./gradlew goJF) and class imports? I'm sorry I typed my suggestions on Github and didn't have the proper formatting and missed an import.

@bradyhunsaker
Copy link
Contributor Author

OK. I'm wrestling with google-java-format right. I'm getting an error like this one:
sherter/google-java-format-gradle-plugin#68

I'll try again tomorrow.

@bradyhunsaker
Copy link
Contributor Author

Thanks. I managed to run google-java-format from a separate download rather than using gradlew. Of course it just made the change you gave.

…rentStationValidator.java

Co-authored-by: David Gamez <1192523+davidgamez@users.noreply.github.com>
@bradyhunsaker
Copy link
Contributor Author

Thanks for the documentation fix.

The end-to-end test is now failing in validate_gradle_wrapper, but the error message looks like it's something with the system itself rather than the PR. I'm not familiar with this testing setup, so I could be wrong about that.

@davidgamez
Copy link
Member

Thanks for the documentation fix.

The end-to-end test is now failing in validate_gradle_wrapper, but the error message looks like it's something with the system itself rather than the PR. I'm not familiar with this testing setup, so I could be wrong about that.

This is an intermittent issue downloading the gradle binary, I retried it and passed without issues. I'm approving and merging right now. Thanks for taking it to the end of the line!!

@davidgamez davidgamez merged commit eada0f4 into MobilityData:master Jun 21, 2023
334 checks passed
@welcome
Copy link

welcome bot commented Jun 21, 2023

🥳 Congrats on getting your first pull request merged!

@bradyhunsaker bradyhunsaker deleted the stations branch June 23, 2023 03:09
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.

None yet

2 participants