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

Add support for national buses and ferries of Nicaragua #130

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Add support for national buses and ferries of Nicaragua #130

wants to merge 1 commit into from

Conversation

ialokim
Copy link
Contributor

@ialokim ialokim commented Feb 16, 2018

This PR only adds the minimal config.json used by the standard creators. However, to successfully produce the countrywide GTFS, it depends on #114, #124, #125, #126 and (to clean the data up) on #127.

Those PRs should be merged first.

@pantierra
Copy link
Contributor

Yeah, nice progress here! I would like to see #83 going in before also. This list of providers is getting long now.

@ialokim ialokim changed the title Add support for national buses of Nicaragua Add support for national buses and ferries of Nicaragua Feb 28, 2018
@ialokim
Copy link
Contributor Author

ialokim commented Feb 28, 2018

I've changed the configuration to allow ferries, too (simply removed route=bus). So now, the ferries from and to Ometepe (e.g. this one) are available inside the resulting GTFS.

@pantierra
Copy link
Contributor

This PR needs to be rebased on the latest commits to the code and modified to stick to the newly introduced naming conventions (see #83 for more information) for selectors.

@ialokim
Copy link
Contributor Author

ialokim commented Mar 1, 2018

This PR needs to be rebased on the latest commits to the code and modified to stick to the newly introduced naming conventions (see #83 for more information) for selectors.

Done. I've renamed the selector to ni_national as suggested in #83.

Copy link
Contributor

@pantierra pantierra 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 to me! All depending PRs have been accepted. So this is probably ready to be merged.
Cool, that there is now support for ferries!!

@pantierra pantierra added the ready label Mar 1, 2018
@pantierra
Copy link
Contributor

pantierra commented Mar 2, 2018

I suggest to include tests for this provider first. Based on the structure of #133 it's quite easy to create them (see #136).

@pantierra pantierra removed the ready label Mar 3, 2018
@pantierra pantierra mentioned this pull request Apr 3, 2018
@grote
Copy link
Owner

grote commented Aug 4, 2018

@xamanu so you don't want to have this merged until there's tests for it?

Copy link
Contributor

@pantierra pantierra left a comment

Choose a reason for hiding this comment

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

Yes, indeed, In my opinion every (new) creator should now include tests.
This will be a good reference implementation for a new city, based on the standard creators.

@pantierra
Copy link
Contributor

In case #136 gets merged in, I'd be happy to provide the missing tests here. Just waiting for the direction we are taking there.

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.

3 participants