[tech] simpler gtfs reading api #780

antoine-de · 2021-05-07T16:58:44Z

closes #739 and preparation adding a bit of docs.

Hide the Configuration needed to read GTFS to have easier hello world and an api closer to the ntfs module.

Also add a gtfs::read method that detect if the GTFS is a zip or not (like ntfs::read).

now the canonimal way to read a gtfs is:

let model = transit_model::gtfs::read("path to a zip or directory")?;

For better control there is also:

let model = transit_model::gtfs::read_from_path("path to a directory")?;

let model = transit_model::gtfs::read_from_zip("path to a zip")?;

or even a gtfs::from_read method that takes a reader (cf doc).

When a configuration is needed, a Reader struct needs to be instanciated

let conf = gtfs::Configuration {
            read_as_line: true,
            ..Default::default()
        };
let model = transit_model::gtfs::Reader::new(conf).from(input_dir)?;

Note: I did not use parse_from_* like it was discussed previously because I think this naming is more consistent with the NTFS.

closes #739 hide the `Configuration` needed to read GTFS to have easier hello world and an api closer to the ntfs module. Also add a `gtfs::read` method that detect if the GTFS is a zip or not (like `ntfs::read`). now the canonimal way to read a gtfs is: ```rust let model = transit_model::gtfs::read("path to a zip or directory")?; ``` For better control there is also: ```rust let model = transit_model::gtfs::read_from_path("path to a directory")?; ``` ```rust let model = transit_model::gtfs::read_from_zip("path to a zip")?; ``` or even a `gtfs::from_read` method that takes a reader (cf doc). When a configuration is need a `Reader` struct is needed ```rust let conf = gtfs::Configuration { read_as_line: true, ..Default::default() }; let model = transit_model::gtfs::Reader::new(conf).from(input_dir)?; ```

antoine-de · 2021-05-10T09:32:34Z

damn clippy doesn't like Reader::default().from_path(.) 😢

antoine-de · 2021-05-10T09:47:22Z

I feel the from_ not so bad since they are in a gtfs::Reader struct, so I felt they were quite understandable, but clippy (and @datanel ) does not agree with me 😄

I feel we have several choices (feel free to add more if I missed some):

1 keep it this way

ignore clippy warning

2 use parse in struct as proposed

2a and don't change free functions

this way we are coherent with ntfs::read method

    reader.parse_zip_read()
    reader.parse_zip_path()
    reader.parse_dir_path()
    reader.parse()
// and
transit_model::gtfs::read()
transit_model::gtfs::from_zip()
transit_model::gtfs::from_path()
transit_model::gtfs::from_zip_read()

2b and change free functions

And should we change the ntfs free functions for coherence?

    reader.parse_zip_read()
    reader.parse_zip_path()
    reader.parse_dir_path()
    reader.parse()
// and
transit_model::gtfs::parse()
transit_model::gtfs::parse_zip_path()
transit_model::gtfs::parse_dir_path()
transit_model::gtfs::parse_zip_read()

Conclusion

I'd vote for 1. or 2a. since they are easy to implement and I feel the api is nice, but I'm open to suggestion

woshilapin

Nice refactorization. For your question, I'd vote 2a too. One is a builder pattern and the other are free functions so I feel they can be named differently. Note that I see one weird thing: we have reader.parse_zip_read() but we have transit_model::gtfs::from_read(). Should we have transit_model::gtfs::from_zip_read()?

woshilapin · 2021-05-10T10:38:29Z

src/gtfs/mod.rs

-// let resp = reqwest::blocking::get(url)?; // or async call
-// let data = std::io::Cursor::new(resp.bytes()?.to_vec());
-// let model = transit_model::gtfs::from_read(data, &url, configuration)?;
+/// ```ignore


Why do we need to ignore it? Because reqwest is not part of the dependency-tree? Why didn't it fail before?

For this specific documentation example, we might be able to find another example that doesn't require to bring a big dependency?

yes reqwest is not a dependency, it cannot be build.
Before the was only 2 / so it was not build at all 😅
Do you have an example in mind? It's true that it's a shame not to build all examples, but I think it can be nice to have a real use case here.

Opened a PR targeting this branch to explore activation of doc test: #782
Let you take only what's fine out of it.

Bringing reqwest will considerably increase the build time as it pull an important number of transitive dependencies. This is one of the reason reqwest was removed from transit_model in the first place. So even if what you propose does work, I'm not sure we want to go that way.

Also, it does bring a dependency to the network inside the tests, I'm not entirely sure if it's risky or not but usually, a test that does not depend on the outside is more reliable.

OK, locally I didn't hit the reqwest build time (but it looks like CI doesn't have it so easy).
Added to network and to the files that would move, it's a pain indeed.

Should we add only the transit_model:: for Errors? (and the complete output line for ntfs doc-test)?

antoine-de · 2021-05-10T13:01:17Z

ok for transit_model::gtfs::from_zip_read().

Is it ok for you if I also change the ntfs::from_read too?

antoine-de · 2021-05-10T16:21:39Z

after some discussion the methods are:

and

(to lazy to copy it and I think the doc is easier to read)

The ntfs module have been changed accordingly:

ntfs::from_read -> ntfs::from_zip_reader
ntfs::read_from_path -> ntfs::from_dir
ntfs::read_from_zip -> ntfs::from_zip`

src/gtfs/mod.rs

woshilapin

Why parse_from_zip_reader()? I thought parse_zip_reader() was fine and a bit less a mouthful.

antoine-de · 2021-05-17T08:00:32Z

isn't parse_from_zip_reader more consistent with the other function (parse_from_zip, parse_from_path)?

or we can remove all the from if you prefer (fine for me)

woshilapin · 2021-05-17T09:07:58Z

I'd be OK to remove from_ everywhere.

antoine-de · 2021-05-17T09:17:24Z

ok, last call I hope 😅

I only changed the Reader functions:

reader.parse(..)
reader.parse_zip(..)
reader.parse_dir(..)
reader.parse_zip_reader(.., ...)

pbougue

Fine refactor, did not review all thoroughly but 👍 from me.

tests/read_gtfs.rs

src/gtfs/mod.rs

pbougue · 2021-05-18T13:34:43Z

src/gtfs/mod.rs

-// let resp = reqwest::blocking::get(url)?; // or async call
-// let data = std::io::Cursor::new(resp.bytes()?.to_vec());
-// let model = transit_model::gtfs::from_read(data, &url, configuration)?;
+/// ```ignore


Opened a PR targeting this branch to explore activation of doc test: #782
Let you take only what's fine out of it.

Co-authored-by: Pierre-Etienne Bougué <pierre-etienne.bougue@kisio.com>

pbougue

Fine for me, pick what you want (if anything) from my PR and close it 👍

antoine-de added 3 commits May 7, 2021 16:59

correction of a doc mistake

4051954

add some tests

9aa6e78

woshilapin previously approved these changes May 10, 2021

View reviewed changes

antoine-de dismissed woshilapin’s stale review via 311652d May 10, 2021 16:05

rename function after review

b363c4d

antoine-de force-pushed the simplier_gtfs_api branch from 311652d to b363c4d Compare May 10, 2021 16:06

datanel previously approved these changes May 10, 2021

View reviewed changes

src/gtfs/mod.rs Outdated Show resolved Hide resolved

woshilapin previously approved these changes May 10, 2021

View reviewed changes

rename ziped to zipped

c61bb1f

antoine-de dismissed stale reviews from woshilapin and datanel via c61bb1f May 17, 2021 08:29

remove from_ from Reader

583f3e8

datanel previously approved these changes May 18, 2021

View reviewed changes

pbougue self-requested a review May 18, 2021 13:08

pbougue previously approved these changes May 18, 2021

View reviewed changes

Update tests/read_gtfs.rs

a1c89c9

Co-authored-by: Pierre-Etienne Bougué <pierre-etienne.bougue@kisio.com>

antoine-de dismissed stale reviews from pbougue and datanel via a1c89c9 May 18, 2021 13:41

pbougue previously approved these changes May 18, 2021

View reviewed changes

fix import in doc test

52fbfa9

antoine-de dismissed pbougue’s stale review via 52fbfa9 May 18, 2021 14:08

datanel approved these changes May 18, 2021

View reviewed changes

pbougue approved these changes May 18, 2021

View reviewed changes

datanel merged commit ab1ec66 into master May 19, 2021

datanel deleted the simplier_gtfs_api branch May 19, 2021 07:44

antoine-de mentioned this pull request Jul 1, 2021

Allow not to parse stop_times rust-transit/gtfs-structure#100

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[tech] simpler gtfs reading api #780

[tech] simpler gtfs reading api #780

antoine-de commented May 7, 2021

antoine-de commented May 10, 2021

antoine-de commented May 10, 2021 •

edited

Loading

woshilapin left a comment

woshilapin May 10, 2021

antoine-de May 10, 2021

pbougue May 18, 2021

woshilapin May 18, 2021

pbougue May 18, 2021 •

edited

Loading

antoine-de commented May 10, 2021

antoine-de commented May 10, 2021

woshilapin left a comment

antoine-de commented May 17, 2021 •

edited

Loading

woshilapin commented May 17, 2021

antoine-de commented May 17, 2021

pbougue left a comment

pbougue May 18, 2021

pbougue left a comment

[tech] simpler gtfs reading api #780

[tech] simpler gtfs reading api #780

Conversation

antoine-de commented May 7, 2021

antoine-de commented May 10, 2021

antoine-de commented May 10, 2021 • edited Loading

1 keep it this way

2 use parse in struct as proposed

2a and don't change free functions

2b and change free functions

Conclusion

woshilapin left a comment

Choose a reason for hiding this comment

woshilapin May 10, 2021

Choose a reason for hiding this comment

antoine-de May 10, 2021

Choose a reason for hiding this comment

pbougue May 18, 2021

Choose a reason for hiding this comment

woshilapin May 18, 2021

Choose a reason for hiding this comment

pbougue May 18, 2021 • edited Loading

Choose a reason for hiding this comment

antoine-de commented May 10, 2021

antoine-de commented May 10, 2021

woshilapin left a comment

Choose a reason for hiding this comment

antoine-de commented May 17, 2021 • edited Loading

woshilapin commented May 17, 2021

antoine-de commented May 17, 2021

pbougue left a comment

Choose a reason for hiding this comment

pbougue May 18, 2021

Choose a reason for hiding this comment

pbougue left a comment

Choose a reason for hiding this comment

antoine-de commented May 10, 2021 •

edited

Loading

pbougue May 18, 2021 •

edited

Loading

antoine-de commented May 17, 2021 •

edited

Loading