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

[feature] gtfs2ntfs: read route_desc into comments #733

Merged
merged 3 commits into from
Jan 27, 2021
Merged

[feature] gtfs2ntfs: read route_desc into comments #733

merged 3 commits into from
Jan 27, 2021

Conversation

Tristramg
Copy link
Contributor

woshilapin
woshilapin reviewed Jan 27, 2021
View reviewed changes
Copy link
Contributor

@woshilapin woshilapin left a comment

Choose a reason for hiding this comment

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

Wasn't it possible to only add a new comment on existing features instead of adding an entire new set of fixtures?

@Tristramg
Copy link
Contributor Author

Concerning the fixtures, many tests rely on the output of minimal. This would require small changes a bit everywhere. Do you prefer that approach?

@woshilapin
Copy link
Contributor

Concerning the fixtures, many tests rely on the output of minimal. This would require small changes a bit everywhere. Do you prefer that approach?

I see, if the alternative is to modify minimal, then I guess you made it right. Keep what you've done then.

woshilapin
woshilapin previously approved these changes Jan 27, 2021
View reviewed changes
@Tristramg
Copy link
Contributor Author

I added a commit which applies the same behaviour for stop comments.
At this point I’m wondering if we shouldn’t add a trait WithDescription to the GTFS model, but I guess I should leave the rabbit hole for now :(

@Tristramg Tristramg requested a review from woshilapin January 27, 2021 13:50
woshilapin
woshilapin previously approved these changes Jan 27, 2021
View reviewed changes
Copy link
Contributor

@woshilapin woshilapin left a comment

Choose a reason for hiding this comment

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

I'm just wondering if generate_route_comment wouldn't be more accurate as the function name but apart from that, this PR is good to go.

@Tristramg
Copy link
Contributor Author

Thanks, I renamed it

@Tristramg Tristramg merged commit 74e25e0 into master Jan 27, 2021
@Tristramg Tristramg deleted the nd_1075 branch January 27, 2021 14:15
@pbougue pbougue changed the title gtfs2ntfs: read route_desc into comments [feat] gtfs2ntfs: read route_desc into comments Feb 1, 2021
@datanel datanel changed the title [feat] gtfs2ntfs: read route_desc into comments [feature] gtfs2ntfs: read route_desc into comments Feb 1, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@woshilapin woshilapin woshilapin approved these changes

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

Successfully merging this pull request may close these issues.
2 participants
@Tristramg @woshilapin