georust / geozero

Zero-Copy reading and writing of geospatial data.
Apache License 2.0
342 stars 35 forks source link

fix issue processing certain types at start of ShapeRecord #18

Closed jcary741 closed 2 years ago

jcary741 commented 2 years ago

I ran into a situation when reading a shapefile into geojson where, if the first property of a given record is null (and thereby omitted by the processor), a comma is written by the GeoJsonWriter for the next property resulting in invalid JSON like {, "second_property": 123}

This change offsets the i value given to the GeoJsonWriter (or whatever writer) when we're skipping null properties.

pka commented 2 years ago

Thanks for your PR! What do you think about the more verbose variant:

        let mut i = 0;
        for (name, value) in self.record.as_ref().iter() {
            match value {
                FieldValue::Character(Some(val)) => {
                    finish = processor.property(i, name, &ColumnValue::String(val))?;
                    i += 1;
                }
                FieldValue::Numeric(Some(val)) => {
                    finish = processor.property(i, name, &ColumnValue::Double(*val))?;
                    i += 1;
                }
                FieldValue::Logical(Some(val)) => {
                    finish = processor.property(i, name, &ColumnValue::Bool(*val))?;
                    i += 1;
                }
                FieldValue::Date(Some(_)) => {
                    let s = format!("{}", value);
                    finish = processor.property(i, name, &ColumnValue::DateTime(&s))?;
                    i += 1;
                }
                FieldValue::Float(Some(val)) => {
                    finish = processor.property(i, name, &ColumnValue::Float(*val))?;
                    i += 1;
                }
                FieldValue::Integer(val) => {
                    finish = processor.property(i, name, &ColumnValue::Int(*val))?;
                    i += 1;
                }
                FieldValue::Double(val) => {
                    finish = processor.property(i, name, &ColumnValue::Double(*val))?;
                    i += 1;
                }
                FieldValue::Currency(val) => {
                    finish = processor.property(i, name, &ColumnValue::Double(*val))?;
                    i += 1;
                }
                FieldValue::DateTime(_) => {
                    let s = format!("{}", value);
                    finish = processor.property(i, name, &ColumnValue::DateTime(&s))?;
                    i += 1;
                }
                FieldValue::Memo(val) => {
                    finish = processor.property(i, name, &ColumnValue::String(val))?;
                    i += 1;
                }
                FieldValue::Character(None)
                | FieldValue::Numeric(None)
                | FieldValue::Logical(None)
                | FieldValue::Date(None)
                | FieldValue::Float(None) => {} // Ignore NULL values
            }
jcary741 commented 2 years ago

Ha! And here I was just trying to make enumerate work 😛

I took your suggestion one step further (see latest commit) by just assigning the result of the match statement, resulting in overall fewer lines of code.

pka commented 2 years ago

Thanks!