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.

Sign up for GitHub

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

Jump to bottom

Upgrade to last Transitfeed #122

Merged
merged 3 commits into from
Feb 4, 2018
Merged

Upgrade to last Transitfeed #122

merged 3 commits into from
Feb 4, 2018

Conversation

prhod
Copy link
Collaborator

@prhod prhod commented Jan 27, 2018

No description provided.

@prhod prhod mentioned this pull request Jan 27, 2018
Closed
@grote
Copy link
Owner

grote commented Jan 27, 2018

Wow, that is strange! Suddenly everything works!???

However, before upgrading, we should remove the feed-info workaround at least.

@prhod
Copy link
Collaborator Author

prhod commented Jan 27, 2018

The actual test only check for the size of the file, so there could still be something.
I'm adding the do not merge label until I add a test on the OSM relation I noticed before. Sorry for the too early PR !

@prhod
Copy link
Collaborator Author

prhod commented Jan 27, 2018

I added a manual diff on stop_times (to have more explicit messages) and found the bug. It was not related to the upgrade of TransitFeed !
cheers to @nlehuby who told me what the bug could be by reading the code for 10 minutes when I struggled for hours ;)

@grote
Copy link
Owner

grote commented Jan 28, 2018

Thanks @prhod!

Could you please also remove the feed_info workaround for this to get merged? It is essentially just reverting 7101a1f.

@prhod
Copy link
Collaborator Author

prhod commented Jan 30, 2018

Sorry for the delay, I made the revert :)

grote
grote approved these changes Jan 30, 2018
View reviewed changes
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.

Thanks a lot! Looks all good to me.

nlehuby
nlehuby approved these changes Feb 1, 2018
View reviewed changes
Copy link
Collaborator

@nlehuby nlehuby left a comment

Choose a reason for hiding this comment

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

👍

xamanu
xamanu approved these changes Feb 2, 2018
View reviewed changes
Copy link
Contributor

@xamanu xamanu 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.

@xamanu xamanu added the ready label Feb 2, 2018
ialokim
ialokim approved these changes Feb 2, 2018
View reviewed changes
Copy link
Contributor

@ialokim ialokim left a comment

Choose a reason for hiding this comment

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

Looks like ready to be merged :)

@grote grote merged commit 87e0d73 into grote:master Feb 4, 2018
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@xamanu xamanu xamanu approved these changes

@grote grote grote approved these changes

@nlehuby nlehuby nlehuby approved these changes

@ialokim ialokim ialokim approved these changes

Assignees
No one assigned
Labels
ready
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

None yet
5 participants
@prhod @grote @xamanu @nlehuby @ialokim