Feature::field returns Result<Option<FieldValue>>

[dead10ck]

Fixes #132

[michaelkirk]

This seems like a useful change, though it's likely to break a lot of existing code right? Maybe worth a note in CHANGES.md to help people migrating?

[dead10ck]

Yes, it's likely to break a lot of code. I do think it's beneficial, however. I myself thought of this because I found myself writing code like

fn get_property_string(
    key: &str,
    feature: &mut gdal::vector::Feature,
) -> Result<Option<String>, Box<dyn Error + Send + Sync>> {
    let s: String = feature
        .field(key)?
        .into_string()
        .ok_or(format!("missing key: {}", key))?;

    if &s[..] == "" {
        Ok(None)
    } else {
        Ok(Some(s))
    }
}

to get around it. This just so happened to work for me, but I imagine there are use cases where 0 and "" are valid values that need to be distinguished from null.

I'll add a note to the change log.

[michaelkirk]

Are there likely to be people for whom the old behavior, of getting the default value, is desirable?

If so, after this change, how will they restore that behavior?
Will they need to use snippets like: .unwrap_or_default(0) . unwrap_or_default("")? or is there a way to say something like "field.default_value()"?

[michaelkirk]

(To be clear, I think your change is good overall, just trying to think of any pain we can save a minority of people)

[dead10ck]

Yes, that's right, if they had dataset.field("some_field")?.into_int() before, they could do dataset.field("some_field")?.unwrap_or(FieldValue::Integer(0)).into_int(). Although it would surprise me what kind of use case there would be where people wouldn't be surprised by nulls turning into 0 or "". Probably the more likely case is people aren't aware that nulls silently get turned into real values.

[michaelkirk]

Probably the more likely case is people aren't aware that nulls silently get turned into real values.

Yeah, that seems likely. 👍

it would surprise me what kind of use case there would be where people wouldn't be surprised by nulls turning into 0 or "".

I've never personally used this feature, but apparently you can get/set other defaults for fields: https://gdal.org/doxygen/classOGRFieldDefn.html#ac4210fa7c6f10ed090a5558224447cfa

Maybe out of scope for this PR, but since you're thinking about it, what's your opinion on exposing an API leveraging that functionality like:

let foo: Result<FieldValue> = dataset.field_default("some_field");

Then people could exactly mirror the previous behavior with:

let foo: Result<FieldValue> = dataset.field("some_field")?.unwrap_or_else(dataset.field_default("some_field"))

and maybe offered succinctly as:

let foo: Result<FieldValue> = dataset.field_value_or_default("some_field");

edit: fixed code

[dead10ck]

That makes a lot of sense to me. Having a field_value_or_default function is clear and descriptive, and exposes useful functionality from OGR.

And since you ask, I was actually starting to think about whether it would be possible to do away with the FieldValue type completely and just use generics. I haven't fully fleshed out the details, but I was thinking it should be possible to just make the field generic on the return type, so you could just do

let str_var: String = dataset.field("string_field")?.unwrap();
let int_var: i32 = dataset.field("int_field")?.unwrap();
etc...

I think it could be done if we made a Field type that mirrors the OGRFieldDfn type, then impl TryFrom<str> for Field<String/i32/etc>, and then the signature of field could be something like

fn field<T, D>(field_dfn: D) -> Result<Option<T>> where D: TryInto<Field<T>>

The Field type could have that set_default method on it too.

[dead10ck]

Was there anything else you wanted for this PR? Did you want me to add that example code snippet for getting a default value?

[jdroenner]

i think the PR looks fine. We will wait a few days for other PRs with breaking changes before we release a new version.
Could you add docs to the field_from_id method and include that example snippet?

[dead10ck]

Done 🙂

[jdroenner]

uh clippy is not happy :( could you check what is wrong?

[dead10ck]

Oh sorry, I did not run the tests with --all-features, so I missed a breakage in the examples. I've fixed it, and a couple other issues with the examples. I ran all of the example binaries and made sure they worked.