improve contributors compatibility #99

nberard · 2018-06-14T15:04:55Z

Fix #97 and compatibility with several contributors

prhod

Thanks ! I've 2 small comments and a question :)

prhod · 2018-06-15T07:51:16Z

src/gtfs/read.rs

@@ -744,10 +750,11 @@ fn get_modes_from_gtfs(
        .iter()
        .map(|mt| get_commercial_mode(mt))
        .collect();
-    let physical_modes = gtfs_mode_types
+    let physical_modes_set: HashSet<objects::PhysicalMode> = gtfs_mode_types


I think you should remove the explicit type

it has to be there otherwise it gets a vector, we use hashset first to remove duplicate and then transform to a vector for function return

it cannot infer if he removes the type but he can write HashSet<_>

prhod · 2018-06-15T07:51:39Z

src/gtfs/read.rs

        .iter()
        .map(|mt| get_physical_mode(mt))
        .collect();
+    let physical_modes: Vec<objects::PhysicalMode> = physical_modes_set.into_iter().collect();


here yes you can remove Vec<objects::PhysicalMode>

prhod · 2018-06-15T07:54:10Z

src/objects.rs

+    }
+}
+
+impl Eq for PhysicalMode {}


Is it ok and useful to declare impl without any function ?

it has to be there but i guess it uses the declaration from the trait PartialEq

Eq has no extra methods, it is only informing the compiler that this is an equivalence relation rather than a partial equivalence relation.

see https://doc.rust-lang.org/std/cmp/trait.Eq.html

prhod

Thanks for the reply 👍

datanel · 2018-06-15T08:06:46Z

src/gtfs/read.rs

-#[derive(Derivative)]
-#[derivative(Default(bound = ""))]
-#[derive(Serialize, Deserialize, Debug, Clone, PartialEq, Eq, Hash)]
+#[derive(Derivative, Serialize, Deserialize, Debug, Clone, PartialEq, Eq, Hash)]


you can add

#[derivative(Default)]

datanel · 2018-06-15T08:07:03Z

src/gtfs/read.rs

    #[serde(rename = "2")]
    StopEntrace,
 }

+impl Default for StopLocationType {


you can remove this implementation

datanel · 2018-06-15T08:19:42Z

src/gtfs/read.rs

@@ -744,10 +750,11 @@ fn get_modes_from_gtfs(
        .iter()
        .map(|mt| get_commercial_mode(mt))
        .collect();
-    let physical_modes = gtfs_mode_types
+    let physical_modes_set: HashSet<objects::PhysicalMode> = gtfs_mode_types


it cannot infer if he removes the type but he can write HashSet<_>

datanel · 2018-06-15T08:20:21Z

src/gtfs/read.rs

        .iter()
        .map(|mt| get_physical_mode(mt))
        .collect();
+    let physical_modes: Vec<objects::PhysicalMode> = physical_modes_set.into_iter().collect();


here yes you can remove Vec<objects::PhysicalMode>

datanel · 2018-06-15T08:55:13Z

src/gtfs/read.rs

@@ -1446,6 +1454,41 @@ mod tests {
        });
    }

+    #[test]
+    fn gtfs_trips_no_direction_id() {


this test seems similar to test gtfs_trips() you modify it instead of create a new one

as the title says it tests the trips with no column direction_id

ok but in gtfs_trips() there is the same fixture with no direction id.
I thought you might add an assertion.

the trips file with no direction_id for a row does not make the same test as the missing column, that's why i added a new test with a missing column

TeXitoi · 2018-06-18T10:22:13Z

src/gtfs/read.rs

        .iter()
        .map(|mt| get_physical_mode(mt))
        .collect();
+    let physical_modes = physical_modes_set.into_iter().collect();


let physical_modes = gtfs_mode_types .iter() .map(|mt| get_physical_mode(mt)) .collect::<BTreeSet<_>>() .into_iter() .collect();

BTreeSet because it will have a deterministic order.

order by id? or co2_emission?

order by Ord

TeXitoi · 2018-06-18T10:22:36Z

src/gtfs/read.rs

@@ -523,7 +522,7 @@ pub fn read_stops<P: AsRef<path::Path>>(
        let comment_links = manage_comment_from_stop(comments, &stop);
        let equipment_id = get_equipment_id_and_populate_equipments(equipments, &stop);
        match stop.location_type {
-            StopLocationType::StopArea => {
+            StopLocationType::StopPoint => {


TeXitoi · 2018-06-18T10:28:37Z

src/utils.rs

@@ -173,3 +173,15 @@ where
        obj.add_prefix(prefix);
    }
 }
+
+macro_rules! skip_fail {


I don't like this macro: it's too dependent of the context.

should i change the name or just remove the macro?

nberard added 2 commits June 14, 2018 11:57

make gtfs2ntfs non blocking for switzerland dataset

f18b908

add default value for location type if empty

d67f124

prhod reviewed Jun 15, 2018

View reviewed changes

prhod previously approved these changes Jun 15, 2018

View reviewed changes

datanel reviewed Jun 15, 2018

View reviewed changes

nberard dismissed prhod’s stale review via 2065388 June 15, 2018 11:15

nberard force-pushed the fix-96-97-inconsistencies-gtfs2ntfs branch from 079ae8d to 2065388 Compare June 15, 2018 11:15

nberard changed the title ~~fix tartare issue 529~~ improve contributors compatibility Jun 15, 2018

TeXitoi suggested changes Jun 18, 2018

View reviewed changes

nberard force-pushed the fix-96-97-inconsistencies-gtfs2ntfs branch 3 times, most recently from 6d936e4 to bec0150 Compare June 18, 2018 15:17

nberard added 2 commits June 18, 2018 17:18

add default value for direction_id in trips

bec0150

use btreeset for physical modes

d2f58a5

datanel merged commit 696dd4d into hove-io:master Jun 20, 2018

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

improve contributors compatibility #99

improve contributors compatibility #99

nberard commented Jun 14, 2018 •

edited

Loading

prhod left a comment

prhod Jun 15, 2018

nberard Jun 15, 2018

datanel Jun 15, 2018

prhod Jun 15, 2018

datanel Jun 15, 2018

prhod Jun 15, 2018

nberard Jun 15, 2018

prhod left a comment

datanel Jun 15, 2018

nberard Jun 15, 2018

datanel Jun 15, 2018

nberard Jun 15, 2018

datanel Jun 15, 2018

datanel Jun 15, 2018

datanel Jun 15, 2018

nberard Jun 15, 2018 •

edited

Loading

datanel Jun 15, 2018

nberard Jun 18, 2018

TeXitoi Jun 18, 2018

nberard Jun 18, 2018

TeXitoi Jun 18, 2018

TeXitoi Jun 18, 2018

TeXitoi Jun 18, 2018

nberard Jun 18, 2018

improve contributors compatibility #99

improve contributors compatibility #99

Conversation

nberard commented Jun 14, 2018 • edited Loading

prhod left a 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 left a 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

nberard Jun 15, 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

nberard commented Jun 14, 2018 •

edited

Loading

nberard Jun 15, 2018 •

edited

Loading