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: New rule - SingleShapePointValidator for flagging shapes with a single shape point #1753

Merged
merged 6 commits into from
Jun 6, 2024

Conversation

praneethd7
Copy link
Contributor

Summary:

Reference - #1733. Some GTFS feeds have shapes with a single shape point. These route shapes cannot be visualized on any GIS software (which requires LineString) and are therefore indicative of an error in the creation (as a line requires a minimum of two points)

Expected behavior:

The new rule flags such cases with the single_shape_point notice which reports the shape_id and csvRowNumber of the shape with a single shape point(one row in the feed).

Example Feed - LANTA GTFS:
image

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
  • Add or update any needed documentation to the repo
  • 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)

Copy link

welcome bot commented May 9, 2024

Thanks for opening this pull request! You're awesome. We use semantic commit messages to streamline the release process. Before your pull request can be merged, you should update your pull request title to start with a semantic prefix. Examples of titles with semantic prefixes:

  • fix: Bug with ssl network connections + Java module permissions.
  • feat: Initial support for multiple @PrimaryKey annotations.
  • docs: update RELEASE.md with new process
    To get this PR to the finish line, please do the following:
  • Read our Contribution Guidelines
  • Follow Google Java style coding standards
  • Include tests when adding/changing behavior
  • Include screenshots

@CLAassistant
Copy link

CLAassistant commented May 9, 2024

CLA assistant check
All committers have signed the CLA.

@qcdyx qcdyx self-assigned this May 21, 2024
@qcdyx qcdyx requested a review from emmambd May 22, 2024 14:18
@qcdyx
Copy link
Contributor

qcdyx commented May 22, 2024

Hello @praneethd7, thank you for contributing to the GTFS Validator. I have two pieces of feedback:

  1. The Java Formatting check is failing. Could you fix it by running ./gradlew goJF or gradle goJF?
  2. The Rule acceptance test/compare-outputs check is failing. There are 22 new warnings out of 1520 datasets (~1%) that are invalid due to code change, which exceeds the 1% threshold. Could you review these 22 datasets to see if they actually have the single shape point issue?

@praneethd7
Copy link
Contributor Author

Hi @qcdyx! Thank you for your suggestions. I have re-formatted the code according to the Google Java format. Is there a way to know which are those 22 feeds that have this error? Also, I am not sure if I have access to all of them. Please let me know.😄

@qcdyx
Copy link
Contributor

qcdyx commented May 27, 2024

Hello @praneethd7 Yes, you have access to those 22 feeds. To get them, goto https://github.com/MobilityData/gtfs-validator/actions/runs/9197621511?pr=1753, scroll down to bottom, find the [acceptance_test_report] artifact(https://github.com/MobilityData/gtfs-validator/actions/runs/9197621511/artifacts/1541824946), and download it.
image
Unzip acceptance_test_report.zip, open acceptance_test_report.json and search for "sourceUrl", you can click the link and download the dataset.
Don't forget to format code again since we merge master branch into your branch.

@praneethd7
Copy link
Contributor Author

@qcdyx Thank you for helping me with the code formatting. I have manually checked the 22 feeds and they all have single_shape_point error. I have verified the notice counts for each feed and everything seems to be reported correctly. I guess we are good to go :)

@qcdyx
Copy link
Contributor

qcdyx commented Jun 4, 2024

@emmambd Should we consult with the spec team to verify the naming and description of the single_shape_point notice?

/**
* The shape within `shapes.txt` contains a single shape point.
*
* <p>A shape should contain more than one shape point to visualize the route on any GIS software
Copy link
Contributor

Choose a reason for hiding this comment

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

Request to modify description to "A shape should contain more than one shape point to visualize the route" to be more generic.

@emmambd
Copy link
Contributor

emmambd commented Jun 5, 2024

@qcdyx No, that one's for me to review! @praneethd7 The name looks great! I made a minor comment on the description for the sake of being generic.

Copy link
Contributor

@qcdyx qcdyx left a comment

Choose a reason for hiding this comment

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

LGTM - ready to be merged after the modification of rule description

@praneethd7
Copy link
Contributor Author

@emmambd @qcdyx Thank you for the suggestions. Changed the rule description

@qcdyx
Copy link
Contributor

qcdyx commented Jun 6, 2024

@praneethd7 Great, you PR is merged. Thanks again for contributing to GTFS Validator.

@qcdyx qcdyx merged commit edf9867 into MobilityData:master Jun 6, 2024
334 of 335 checks passed
Copy link

welcome bot commented Jun 6, 2024

🥳 Congrats on getting your first pull request merged!

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

5 participants