csv writer #33
csv writer #33
Conversation
a feature. Revert this and run a test to see why I want this. I'm wondering if there are unintended consequences elsewhere though.
|
|
||
| // Writing Wkt is delegated to the WktWriter, and it's output buffered to Vec, to be eventually | ||
| // incorporated in the output of the CSV writer. Is there a better way? | ||
| mod buffering_wkt_writer { |
There was a problem hiding this comment.
And another entire level of delegation!!! 😱
|
|
||
| impl<W: Write> GeomProcessor for CsvWriter<'_, W> { | ||
| fn dimensions(&self) -> CoordDimensions { | ||
| self.wkt_writer.dimensions() |
There was a problem hiding this comment.
All this explicit delegation is kind of tedious. But I'm not sure of a better way.
| ] | ||
| }); | ||
|
|
||
| let expected_output = r#"geometry,address,datetime,incident number,type |
There was a problem hiding this comment.
note the column order here is different from what's listed in the geojson. By default geojson doesn't keep it's properties ordered. The underlying serde_json crate does offers this as an optional feature. I opened an issue to discuss exposing it in geojson here: georust/geojson#187
I'm not totally sure we'd want to use it, but it might be a nice option since we have a concept of "property idx" in some data formats.
| @@ -96,7 +96,7 @@ fn process_geojson<P: FeatureProcessor>(gj: &GeoGeoJson, processor: &mut P) -> R | |||
| } | |||
| if let Some(ref geometry) = feature.geometry { | |||
| processor.geometry_begin()?; | |||
| process_geojson_geom_n(geometry, idx, processor)?; | |||
| process_geojson_geom_n(geometry, 0, processor)?; | |||
There was a problem hiding this comment.
Note this change - It might be controversial!
Without it, the second line of the csv will have it's geometry prefixed with an unwanted ",".
I'm willing to be talked out of this if you have a good rationale.
Alternatively, I could revert this and hack around this in the CSVWriter, but I think it's arguably correct for the first geometry in each feature to have index 0.
|
Whoops - I'm going to close this. The test failures are indicative of another issue that I've been tripping on. |
I encountered this with my initial approach (georust#33), and incorporated a work-around in georust#36, but I failed to consistently apply the work-around, so it was still broken for some geometry types.
I encountered this with my initial approach (georust#33), and incorporated a work-around in georust#36, but I failed to consistently apply the work-around, so it was still broken for some geometry types.
FIXES #24
This is a follow up to #30, but for writing CSV's.
Feel free to ask questions or push back on the implementation - I'm not super happy with it.