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: Added route names component #1469

Merged
merged 3 commits into from
Jun 1, 2023
Merged

feat: Added route names component #1469

merged 3 commits into from
Jun 1, 2023

Conversation

cka-y
Copy link
Contributor

@cka-y cka-y commented May 31, 2023

Summary:

Add a "Route Names" GTFS Component #1464
Closes #1464

Expected behavior:

"Route Names" component should be added to this list. We consider this component present if both the fields route_short_name and route_long_name in routes.txt are present, and if they have at least one value defined.
Valid Route Names Component Report

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)

@welcome
Copy link

welcome bot commented May 31, 2023

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 31, 2023

CLA assistant check
All committers have signed the CLA.

@davidgamez davidgamez requested a review from jcpitre June 1, 2023 13:18
@cka-y cka-y marked this pull request as draft June 1, 2023 13:42
fix: short and long name needs to be defined for the same entry
@cka-y cka-y marked this pull request as ready for review June 1, 2023 13:49
if (routeTable.hasColumn(GtfsRoute.ROUTE_SHORT_NAME_FIELD_NAME)
&& routeTable.hasColumn(GtfsRoute.ROUTE_LONG_NAME_FIELD_NAME))
return routeTable.getEntities().stream()
.anyMatch(route -> route.hasRouteShortName() && route.hasRouteLongName());
Copy link
Contributor

Choose a reason for hiding this comment

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

It's unclear to me if this should be an AND or an OR. The issue is a bit ambiguous on the subject IMO:

We consider this component present if both the fields route_short_name and route_long_name in routes.txt are present, and if they have at least one value defined.

Does that mean that there should be one value for each field, or one value for either one of the fields?
Logically it seems that if you have either a short name or a long name it should be accepted. But it should be clarified.

Copy link
Contributor Author

@cka-y cka-y Jun 1, 2023

Choose a reason for hiding this comment

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

I actually checked with Isabelle regarding that :
Screenshot 2023-06-01 at 11 30 10 AM

@jcpitre
Copy link
Contributor

jcpitre commented Jun 1, 2023

I would suggest briefly explaining in the issue the manual testing you did if any. Did you modify a dataset to simulate the situation? How was it modified? Maybe include the dataset you used? Or simple paste a couple of lines from the modified file form the dataset..

@cka-y cka-y merged commit ead1346 into MobilityData:master Jun 1, 2023
333 checks passed
@welcome
Copy link

welcome bot commented Jun 1, 2023

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

Add a "Route Names" GTFS Component #1463
3 participants