georust / gdal

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

Changed a number of APIs using `isize` when `usize` is semantically more appropriate #497

Closed metasim closed 9 months ago

metasim commented 9 months ago

Scope:

Closes #495

metasim commented 9 months ago

@lnicola

should we also change rasterband and the histogram APIs to return an error?

My apologies, but I'm not following your question... 🤔

lnicola commented 9 months ago

We panic there if the index is too large.

lnicola commented 9 months ago

I meant https://github.com/georust/gdal/blob/master/src/raster/rasterband.rs#L39 and https://github.com/georust/gdal/blob/master/src/raster/rasterband.rs#L1010 etc.

metasim commented 9 months ago

ooooh, yeah... let me fix that. Given the option, I think passing the error up is better.

metasim commented 9 months ago

@lnicola Do I still have your approval after this additional change?

https://github.com/georust/gdal/pull/497/commits/75b4c122fbd2e6c487c6647f0bc8fe65ba08f321