georust / gdal

Rust bindings for GDAL
https://crates.io/crates/gdal
MIT License
359 stars 94 forks source link

`Feature` fields getters and setters should accept field indexes #500

Open lnicola opened 9 months ago

lnicola commented 9 months ago

I was recently looking at a small program that spent 30% of its CPU time in strcasecmp.

To be honest, I wish they'd only take the field index and never the name, but that's might really upset some users.

ChristianBeilschmidt commented 8 months ago

Well, a resolution from name to index is a convenience thing, I guess. There could be two separate methods or one that accepts both. But you definitely need the opportunity to use the names.

So, what is your suggestion here?

lnicola commented 8 months ago

So we can add fn set_field_by_index(&self, idx: usize, value: &FieldValue), fn set_field_integer_by_index(&self, idx: usize, value: i32) and about 9 more.

But I'm worried that users will just do:

for feature in layer.features() {
    feature.set_field_integer("foo", 20);
}

instead of:

let foo_col = layer.field_index("foo")?;
for feature in layer.features() {
    feature.set_field_integer_by_index(foo_col, 20);
}

just because it's easier to find.

ChristianBeilschmidt commented 8 months ago

Yeah, either it would be …by_index and …by_name separately or you just have to keep strongly typing it to size :smile: .