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

Import route_directions.txt, if present #73

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Import route_directions.txt, if present #73

wants to merge 4 commits into from

Conversation

mharvey
Copy link
Contributor

@mharvey mharvey commented Aug 29, 2017

Some providers, notably TriMet, provide a route_directions.txt file that associates a route_id and direction_id with a human-readable name. This data is valuable and can be much more convenient to use than trying to divine this information using StopTime.stop_headsign — especially with the larger feeds.

This PR implements the RouteDirection model and was based largely on existing models.

In addition to passing automated tests, I tested this with a few recent TriMet and C-TRAN feeds (which do not have route_directions.txt) and imports occurred as expected.

Morgan Harvey added 4 commits July 31, 2017 13:43
Begin addition of non-standard route_direction.txt
Finished specification of model for RouteDirection.
Added basic tests and configurations.
Add RouteDirection to feed.py to get route_directions.txt to import. Modified associated tests to account for new optional file.
@coveralls
Copy link

coveralls commented Aug 29, 2017

Coverage Status

Coverage remained the same at 100.0% when pulling a196e3a on mharvey:route_directions into f104c09 on tulsawebdevs:master.

@mharvey
Copy link
Contributor Author

mharvey commented Aug 29, 2017

This adresses #63

Copy link
Member

@jwhitlock jwhitlock left a comment

Choose a reason for hiding this comment

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

The code looks good, and mirrors the standards of existing code nicely. I'd like to do some manual testing before merging. If you know of a feed smaller than TriMet that uses this, I'd appreciate it 😄 .

route = models.ForeignKey(
'Route', null=True, blank=True,
help_text="Route for which this direction description applies.",
on_delete=models.CASCADE)
Copy link
Member

Choose a reason for hiding this comment

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

👍 adding the on_delete method

class RouteDirection(Base):
"""Associate a name with a Route and Direction

Maps to non-standard route_directions.txt in GTFS feed.
Copy link
Member

Choose a reason for hiding this comment

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

Are there any online docs for the format of this file that we could link here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Maybe the best approach would be for me to create a section for it under Optional Files in docs/gtfs.rst? I could do that this afternoon and add to this PR.

The closest thing to documentation seems to be this discussion, which mainly discusses adding this info to trips.txt, but the final paragraph of the final post in that thread explains why a route_directions.txt would be a better approach and seems to be where it was first suggested. Guessing people ran off with it after that.

@mharvey
Copy link
Contributor Author

mharvey commented Aug 30, 2017

If you know of a feed smaller than TriMet that uses this, I'd appreciate it

Sure thing!

Albany: http://transitfeeds.com/p/albany-transit-system/435/latest
Emeryville: https://transitfeeds.com/p/emery-go-round/769/20161026
Petaluma: http://transitfeeds.com/p/petaluma-transit-gtfs/675/latest
Patriots Party Boats: http://transitfeeds.com/p/massdot/648/latest

Those each seem to import in under five seconds. 😄

Copy link
Member

@jwhitlock jwhitlock left a comment

Choose a reason for hiding this comment

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

It looks good. There are some follow-on actions that come to mind:

I think the change is useful without this, so I'd be happy to merge this PR as-is and create issues for the follow-on tasks.

@mharvey
Copy link
Contributor Author

mharvey commented Sep 6, 2017

Sorry, some stuff came up and I got sidetracked from this. I'll block out time to get those follow-on actions added tomorrow and Thursday and commit them to this pull request, unless it works better for you to have them in a separate PR.

@jwhitlock
Copy link
Member

Not a problem, and thanks for the work so far! I think the example app is the most work and the lowest priority, so focus on the other bits when you have time.

@jspetrak
Copy link
Contributor

jspetrak commented Apr 2, 2021

We have reviewed this PR as well as extensible model in #83 and the later solution would be better for including non-standard models into multigtfs.

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.

4 participants