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

#9 - Factory based abstracted schedule creation #29

Merged
merged 1 commit into from
Oct 2, 2016
Merged

#9 - Factory based abstracted schedule creation #29

merged 1 commit into from
Oct 2, 2016

Conversation

xamanu
Copy link
Contributor

@xamanu xamanu commented Oct 2, 2016

This pull request introduces a creator classes for each part of the GTFS feed (agency, feed_info, routes, stops and trips).

A new CreatorFactory allows to overwrite default implementations of each individual creator based on naming convention from the selector property of the config file.

No existing logic has been touched, in order to keep pull requests as small as possible. Discussions about logic should go into the two issues about data structure #30 and refactoring the OsmHelper #31.

There are working default implementations for agency and feed_info because these were already (somehow) generic based on values from the config file.

But there are still no default implementations for routes, stops and trips as their current code exclusively works for the Fenix (Florianópolis) network (issues #26 and #27 will handle this). The default creator classes only define the basic functions that those creator are required to implement.

@xamanu xamanu changed the title #9 - Factory based abstracted schedule creating #9 - Factory based abstracted schedule creation Oct 2, 2016
@grote
Copy link
Owner

grote commented Oct 2, 2016

Thanks for this! I think this step-size is indeed more manageable to merge. Just too bad we lost so much time yesterday when you were flying blind ;)

Did you see my comment on the old PR about the creator directory structure? Would it be nicer to just have one creators folder with a subfolder for each region?

@xamanu
Copy link
Contributor Author

xamanu commented Oct 2, 2016

Oh, I missed this comment. I agree I'd prefer to have a subfolder structure! This is much nicer. I will play around with it.

Copy link
Owner

@grote grote left a comment

Choose a reason for hiding this comment

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

Looks good! Just added very minor comments.

import overpy
import re
import pickle
import transitfeed
Copy link
Owner

Choose a reason for hiding this comment

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

From here on up, the imports are unused.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks! Fixed.

@@ -0,0 +1,10 @@
# coding=utf-8

import sys
Copy link
Owner

Choose a reason for hiding this comment

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

sys is unused

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks! Fixed.

@@ -0,0 +1,204 @@
# coding=utf-8

import os
Copy link
Owner

Choose a reason for hiding this comment

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

os is unused

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks! Fixed.


class StopsCreator(object):

def __init__(self, config):
Copy link
Owner

Choose a reason for hiding this comment

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

missing data object?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks! Fixed.


class TripsCreator(object):

def __init__(self, config):
Copy link
Owner

Choose a reason for hiding this comment

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

missing data object?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Thanks! Fixed.

@xamanu
Copy link
Contributor Author

xamanu commented Oct 2, 2016

Subfolder structure is implemented in last commit 😄

@grote
Copy link
Owner

grote commented Oct 2, 2016

Thanks a lot! Going to merge this now.

If you don't like rebasing hold off any new PR for a moment since I will add a style commit and maybe one other.

@grote grote merged commit c8f76c8 into grote:master Oct 2, 2016
@xamanu
Copy link
Contributor Author

xamanu commented Oct 2, 2016

Very nice! Thanks for reviewing and merging in.

@xamanu xamanu deleted the issue-9 branch October 2, 2016 15:36
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