Reading Netex - First version #92

prhod · 2018-05-21T20:53:26Z

Here is a first version of the reading of netex to have some feedbacks.
There is still work to do:

calendars are empty in oslo
transfers are yet to be read
documentation is missing
Netex is read from examples, checking with official documentation will be done afterward
Those points will be done in an other PR.

datanel · 2018-05-22T07:41:37Z

README.md

-navitia_model is a rust crate to manage transit data. Its model corresponds to the one used in [navitia](https://github.com/CanalTP/navitia). This repository also provides (incomplete) [GTFS](http://gtfs.org/) to [NTFS](https://github.com/CanalTP/navitia/blob/dev/documentation/ntfs/ntfs_fr.md) and (soon) NTFS to GTFS conversion.
+navitia_model is a rust crate to manage transit data. Its model corresponds to the one used in [navitia](https://github.com/CanalTP/navitia). This repository also provides :
+- (incomplete) [GTFS](http://gtfs.org/) to [NTFS](https://github.com/CanalTP/navitia/blob/dev/documentation/ntfs/ntfs_fr.md) and (soon) NTFS to GTFS conversion.
+- (incomplete) [Netex](http://netex-cen.eu/) to [NTFS](https://github.com/CanalTP/navitia/blob/dev/documentation/ntfs/ntfs_fr.md) and (soon) Netex to GTFS conversion. Conversion fixtues comes from official [Github repository](https://github.com/NeTEx-CEN/NeTEx/).


datanel · 2018-05-22T07:46:02Z

src/netex/mod.rs

+extern crate tempdir;
+extern crate zip;
+
+fn add_prefix(prefix: String, collections: &mut Collections) -> Result<()> {


this code is duplicated (from gtfs/mod.rs)
We should move it to another place

i've created a read_utils mod for those functions. I needed to change the result of the fn read_config. Tell me if you're not comfortable with it.

datanel · 2018-05-22T07:51:49Z

src/netex/mod.rs

+    println!("Reading Netex data from {:?}", path);
+    let mut collections = Collections::default();
+    if path.is_file() {
+        match path.extension().unwrap().to_str().unwrap() {


you have 2 unwrap. are you sure this never returns None?

The use will be on .xml files or .zip files, so the extension() will always return something. the to_str() will always be successful if the string is UTF-8, and I assume it will always be OK.

and if there is a file without extension?

match path.extension() .ok_or_else(|| format_err!("input file {:?} has no extention", path))? .to_str() .ok_or_else(|| format_err!("input file {:?} has unknown extention", path))? {

Or maybe

match path.extension().and_then(|ext| ext.to_str()) {

and match on the Option.

datanel · 2018-05-22T07:58:04Z

src/netex/mod.rs

+                let mut zip = zip::ZipArchive::new(zip_file)?;
+                for i in 0..zip.len() {
+                    let mut file = zip.by_index(i)?;
+                    if file.sanitized_name().extension().unwrap() == "xml" {


same here file.sanitized_name().extension() could be None

i've added a check

datanel · 2018-05-22T08:01:01Z

src/ntfs/mod.rs

@@ -170,6 +172,18 @@ pub fn write<P: AsRef<path::Path>>(model: &Model, path: P) -> Result<()> {
    Ok(())
 }

+/// Exports a `Model` to a
+/// [NTFS](https://github.com/CanalTP/navitia/blob/dev/documentation/ntfs/ntfs_fr.md)
+/// ZIP archiveat the given full path.


TeXitoi · 2018-05-25T14:33:24Z

Cargo.toml

@@ -14,6 +14,11 @@ serde_derive = "1"
 structopt = "0.2"
 failure = "0.1.1"
 serde_json = "1"
+quick-xml = "0.12.1"
+minidom = "*"


no * dependencies please.

TeXitoi · 2018-05-25T14:43:59Z

src/collection.rs

+    /// # }
+    /// # fn main() { run().unwrap() }
+    /// ```
+    pub fn append(&mut self, other: &mut Vec<T>) -> Result<Idx<T>> {


The idx is strange. Better to just return Result<()> to be consistent with the append of Collection.

TeXitoi · 2018-05-25T14:45:06Z

src/collection.rs

+            let i = other.pop();
+            match i {
+                None => break,
+                Some(item) => idx = self.push(item),


If there is a failure, and then a success, you'll quiet the error. Please add a test of this case.

TeXitoi · 2018-05-25T14:46:11Z

src/collection.rs

+    /// ```
+    pub fn append(&mut self, other: &mut Vec<T>) -> Result<Idx<T>> {
+        if other.is_empty() {
+            bail!("provided Vec is empty")


We don't care of that, it must be legal.

TeXitoi · 2018-05-25T14:53:45Z

src/collection.rs

+                Some(item) => idx = self.push(item),
+            };
+        }
+        idx


pub fn append(&mut self, other: &mut Vec<T>) -> Result<()> { for o in v.drain(..) { let _ = self.push(item)? } Ok(()) }

Thanks for the code, it's much better 👍

TeXitoi · 2018-05-25T16:26:59Z

src/netex/read.rs

+        trip_property_id: None,
+        geometry_id: None,
+        stop_times: vec![],
+    };


The VehicleJourney { ..Default::default() } should be great in this case.

TeXitoi · 2018-05-25T16:47:51Z

src/netex/read.rs

+    calls: &Element,
+) {
+    let mut stop_sequence = 0;
+    for call in calls.children() {


for (stop_sequence, call) in calls.children().enumerate() {

TeXitoi · 2018-05-25T17:07:02Z

src/netex/read.rs

+        phone: None,
+        address: None,
+        sort_order: None,
+    }


..Default::default()

TeXitoi · 2018-05-25T17:11:44Z

src/netex/read.rs

+        "tram" => "Tramway".to_string(),
+        "water" => "Boat".to_string(),
+        "cableWay" => "BusRapidTransit".to_string(),
+        _ => "Bus".to_string(),


TeXitoi · 2018-05-25T17:13:44Z

src/netex/read.rs

+            destination_id: None,
+        };
+        collections.routes.push(route).unwrap();
+    }


..Default::default()

TeXitoi · 2018-05-26T08:00:40Z

src/ntfs/mod.rs

+pub fn write_to_zip<P: AsRef<path::Path>>(model: &Model, path: P) -> Result<()> {
+    let path = path.as_ref();
+    info!("Writing NTFS to ZIP File {:?}", path);
+    let input_tmp_dir = TempDir::new("read_netex_work").unwrap();


no unwrap, simply ?

TeXitoi · 2018-05-26T08:01:01Z

src/ntfs/mod.rs

+    write(model, input_tmp_dir.path())?;
+    zip_to(input_tmp_dir.path(), path)?;
+    Ok(())
+}


Can't you do that without temp dir, directly in the zip file?

I thought about this, but trying to achieve this would implies a heavy refacto and/or a lot of time to do it. The main purpose of this PR is to enable a Netex reading. It could be an enhancement on a further PR (issue #93).

TeXitoi · 2018-05-27T08:22:23Z

src/utils.rs

+    let mut zip = zip::ZipWriter::new(file);
+    let options = zip::write::FileOptions::default()
+        .compression_method(zip::CompressionMethod::Bzip2)
+        .unix_permissions(0o755);


Default permissions should be OK, and 0o644 are better permissions for a zip file.

you're right, i'm removing permissions

TeXitoi · 2018-05-27T08:23:42Z

src/utils.rs

+
+    let mut zip = zip::ZipWriter::new(file);
+    let options = zip::write::FileOptions::default()
+        .compression_method(zip::CompressionMethod::Bzip2)


Will it work out of the box on Windows? Better to use a standard compression algorithm.

I'm not confident with compression algorithm. I totally agree with you. Which one is the most suited ?

TeXitoi · 2018-05-27T08:24:37Z

src/utils.rs

+            .to_string();
+
+        if path.is_file() {
+            println!("adding {:?} as {:?} ...", path, name);


info! or debug!

TeXitoi · 2018-05-27T08:45:03Z

tests/read_netex.rs

+        file1.read_to_end(&mut file1_content)?;
+        file2.read_to_end(&mut file2_content)?;
+        let compare = file1_content == file2_content; //to avoid having file content in std output
+        assert!(compare, "content of file {} is different", file1.name());


You can assert!(a == b, "msg") it will not output file content.

TeXitoi · 2018-05-27T08:56:38Z

tests/read_netex.rs

+    let tmp_dir = TempDir::new("netex_computed_result").unwrap();
+    let file_path = tmp_dir.path().join("netex_computed_result_ratp.zip");
+    // let tmp_dir = Path::new("fixtures/netex/computed_result/");
+    // let file_path = tmp_dir.join("netex_computed_result_ratp.zip");


useless comments?

until stabilized, I use those lines to generate the expected result in a local dir instead of the tmp_dir. I'm removing them ^^

TeXitoi · 2018-05-27T09:00:43Z

tests/read_netex.rs

+    let tmp_dir = TempDir::new("netex_computed_result").unwrap();
+    let file_path = tmp_dir.path().join("netex_computed_result_oslo.zip");
+    // let tmp_dir = Path::new("fixtures/netex/computed_result/");
+    // let file_path = tmp_dir.join("netex_computed_result_oslo.zip");


pbougue · 2018-07-30T09:18:56Z

This was merged little by little, we can now close it.

Pascal Rhod added 11 commits May 21, 2018 21:01

starting netex read

067f460

read basic netex with context

297f6aa

netex - adding errors (WIP)

391890d

netex - reading zip for tests and several enhancements

3fdf3ca

netex - add oslo tests

070f15e

fmt

5013ce9

netex - add tests and removing unzipping to tempdir

a07bc45

fmt

2663d45

netex - starting validitypattern

e99acd9

netex - reading calendars

cd7255e

fmt

d0274d7

datanel reviewed May 22, 2018

View reviewed changes

Pascal Rhod added 4 commits May 23, 2018 12:34

read netex - fix revue

7f42f3c

fix netex tests and some refacto

9ad3df6

fmt

67c1e5d

fix typo

8be243e

TeXitoi reviewed May 25, 2018

View reviewed changes

TeXitoi suggested changes May 27, 2018

View reviewed changes

Pascal Rhod added 4 commits May 29, 2018 07:04

netex - fixing some revue errors

000e161

netex - removing some unwrap

24a6813

netex - improve tests and unzip with revue

8039ac0

netex - fixing some review comments

051181a

prhod added the do_not_merge label Jun 4, 2018

netex - fix some PR comments

6b36852

prhod mentioned this pull request Jun 4, 2018

netex - creating netex2ntfs bin without tests #94

Merged

pbougue closed this Jul 30, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Reading Netex - First version #92

Reading Netex - First version #92

prhod commented May 21, 2018

datanel May 22, 2018

datanel May 22, 2018

prhod May 23, 2018

datanel May 22, 2018

prhod May 23, 2018

datanel May 24, 2018

TeXitoi May 25, 2018

datanel May 22, 2018

prhod May 23, 2018

datanel May 22, 2018

TeXitoi May 25, 2018

TeXitoi May 25, 2018

TeXitoi May 25, 2018

TeXitoi May 25, 2018

TeXitoi May 25, 2018

prhod May 29, 2018

TeXitoi May 25, 2018

TeXitoi May 25, 2018

TeXitoi May 25, 2018

TeXitoi May 25, 2018

TeXitoi May 25, 2018

TeXitoi May 26, 2018

TeXitoi May 26, 2018

prhod May 29, 2018 •

edited

Loading

TeXitoi May 27, 2018

prhod May 29, 2018

TeXitoi May 27, 2018

prhod May 29, 2018

TeXitoi May 27, 2018

TeXitoi May 27, 2018

TeXitoi May 27, 2018

prhod May 29, 2018

TeXitoi May 27, 2018

pbougue commented Jul 30, 2018

Reading Netex - First version #92

Reading Netex - First version #92

Conversation

prhod commented May 21, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

prhod May 29, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

pbougue commented Jul 30, 2018

prhod May 29, 2018 •

edited

Loading