-
Notifications
You must be signed in to change notification settings - Fork 37
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
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
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.