Fallible properties getter

[michaelkirk]

Signature to get a property is:

fn property<T: PropertyReadType>(&self, name: &str) -> Option<T>

But that doesn't allow you to distinguish between "property missing" and "invalid property" (and an error with details about why it's invalid).

I guess I was expecting some kind of Result rather than Option. Is that something you'd consider?

e.g. trying to parse a string field into an integer.

{
  "type": "Feature",
  "geometry": {
    "type": "Point",
    "coordinates": [125.6, 10.1]
  },
  "properties": {
    "foo": "Dinagat Islands"
  }
}

Maybe something like this:

let foo: u64;
match feature.properties::<u64>("foo") {
  Ok(Some(v)) => foo = v,
  Ok(None) => println!("missing property"),
  Err(e) => println!("invalid property: {}", e),
}

[pka]

I didn't consider this case. And I fear, that it could get more complicated for formats supporting an explict NULL value. I think in JSON world you would consider a missing property the same as a null entry?

[michaelkirk]

In the world I was envisioning, I thought a null in js would be like:

Ok(Some(JsonValue::Null))

Which would be different from a missing property:

Ok(None)

Given:

{
  "type": "Feature",
  "geometry": {
    "type": "Point",
    "coordinates": [125.6, 10.1]
  },
  "properties": {
    "foo": "Dinagat Islands"
    "bar": null
  }
}

Trying to parse out a JSON value when null is assigned:

feature.property::<u32>("foo") //  => Ok(Err("Expected a u32 value but found `String`"))
feature.property::<u32>("bar") //  => Ok(Err("Expected a u32 value but found `null`"))
feature.property::<u32>("baz") //  => Ok(None)

I haven't looked deeply, but my assumption was that trying to parse a property into an incompatible type currently returns None. From a debugging perspective it'd be much nicer to have something like Err("expected a number, but found a string").

My hope is that people who want to maintain something like the existing usage would update their callsites to feature.property::<u64>("foo").unwrap_or(None)

An alternative would be to leave the existing API as it is, and implement a parallel try version:

feature.property::<u32>("foo") //  => None
feature.property::<u32>("bar") //  => None
feature.property::<u32>("baz") //  => None
feature.try_property::<u32>("foo") //  => Ok(Err("Expected a u32 value but found `String`"))
feature.try_property::<u32>("bar") //  => Ok(Err("Expected a u32 value but found `null`"))
feature.try_property::<u32>("baz") //  => Ok(None)

Or have the "safe" explicit flavor be default, and let the people who want to YOLO type in a longer name like:

feature.property::<u32>("foo") //  => Ok(Err("Expected a u32 value but found `String`"))
feature.property::<u32>("bar") //  => Ok(Err("Expected a u32 value but found `null`"))
feature.property::<u32>("baz") //  => Ok(None)
feature.property_or_none::<u32>("foo") //  => None
feature.property_or_none::<u32>("bar") //  => None
feature.property_or_none::<u32>("baz") //  => None

[michaelkirk]

Reflecting a bit, maybe this one in particular is weird:

feature.property::<u32>("bar") //  => Ok(Err("Expected a u32 value but found `null`"))

I could see people reasonably expecting:

feature.property::<u32>("bar") //  => Ok(None)

[pka]

Has been implemented in the meantime https://docs.rs/geozero/0.6.0/geozero/trait.FeatureProperties.html#method.property

[michaelkirk]

Following your link, I don't see where this changed.

It looks like we still return Option::None rather than Result::Err if the types don't match.

 fn property<T: PropertyReadType>(&self, name: &str) -> Option<T> {
        let mut reader = PropertyReader { name, value: None };
        if self.process_properties(&mut reader).is_ok() {
            reader.value
        } else {
            None
        }
    }

[pka]

Sorry, I got into a issue closing fever...

[pka]

geozero 0.8.0 has a new property getter signature:

fn property<T: PropertyReadType>(&self, name: &str) -> Result<T>

which has different errors for missing properties and properties with an unexpected type:

GeozeroError::ColumnNotFound(String), // "column `{0}` not found"
GeozeroError::ColumnType(String, String), // "expected a `{0}` value but found `{1}`"

[michaelkirk]

Thanks!