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

[feature] read gtfs route as line #724

Merged
merged 1 commit into from
Jan 5, 2021
Merged

[feature] read gtfs route as line #724

merged 1 commit into from
Jan 5, 2021

Conversation

patochectp
Copy link
Member

ref: ND-1090

antoine-de
antoine-de previously approved these changes Dec 23, 2020
Copy link
Contributor

@antoine-de antoine-de left a comment

Choose a reason for hiding this comment

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

great! ❤️

I added 2 very minor comments feel free to skip them

antoine-de
antoine-de previously approved these changes Dec 23, 2020
@woshilapin woshilapin self-requested a review January 4, 2021 12:30
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.

A few missing things:

src/gtfs/mod.rs Outdated
pub read_as_line: bool,
}

impl Default for Configuration {
Copy link
Contributor

Choose a reason for hiding this comment

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

I believe you can just derive the Default implementation with

#[derive(Default)]

since all you have are the default values (false is the default for bool if I remember correctly).

Copy link
Member Author

Choose a reason for hiding this comment

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

done in f33d555

src/gtfs/read.rs Outdated
@@ -3297,4 +3307,103 @@ mod tests {
);
});
}

#[test]
fn read_gtfs_routes_as_line() {
Copy link
Contributor

Choose a reason for hiding this comment

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

I do not see any generated-UUID in this test. Do we test this?

@codecov
Copy link

codecov bot commented Jan 5, 2021

Codecov Report

Merging #724 (650baea) into master (5efe103) will not change coverage.
The diff coverage is n/a.

Impacted file tree graph

@@      Coverage Diff      @@
##   master   #724   +/-   ##
=============================
=============================

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 5efe103...d606729. Read the comment docs.

datanel
datanel previously approved these changes Jan 5, 2021
woshilapin
woshilapin previously approved these changes Jan 5, 2021
@patochectp patochectp dismissed stale reviews from woshilapin and datanel via d606729 January 5, 2021 16:37
@patochectp patochectp merged commit 8503dd2 into master Jan 5, 2021
@patochectp patochectp deleted the read_gtfs_as_line branch January 5, 2021 16:49
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