Skip to content

Handling cases where feature may not have any geometry at all - #349

Merged
bors[bot] merged 4 commits into
georust:masterfrom
phayes:master
Dec 5, 2022
Merged

bors[bot] merged 4 commits into
georust:masterfrom
phayes:master

Conversation

@phayes

@phayes phayes commented Dec 5, 2022 •

Copy link
Copy Markdown
Contributor
  • [✓] I agree to follow the project's code of conduct.
  • [✓] I added an entry to CHANGES.md if knowledge of this change could be valuable to users.

I've run into a few instances where I'm loading data uploaded from users (for example CSV files) where GDAL isn't able to find any geometry. In instances where you might be working with user-provided data with no geometry, calls to Feature::geometry() simply panic.

This PR documents renders Feature::geometry non-panicking by returning an Option<&Geometry>

@lnicola

lnicola commented Dec 5, 2022 •

Copy link
Copy Markdown
Member

It looks like GDAL returns nullptr for these. I think I'd prefer returning an Option<&Geometry> here. It feel like the "right thing" to do, instead of expecting the user to check beforehand.

@phayes

phayes commented Dec 5, 2022

Copy link
Copy Markdown
Contributor Author

@lnicola ,

I was wondering about returning an Option<&Geometry>. In 99.9% of cases, you're pretty much always working with data that has geometry, so I was feeling uncertain about forcing all users to unwrap and handle an Option instead of just documenting the panic and providing a has_geometry guard function.

I'd be happy to rework this into returning an Option if that's the consensus.

@lnicola

lnicola commented Dec 5, 2022

Copy link
Copy Markdown
Member

Agreed, but in more than 99.9% of cases, writing to a file will succeed, yet Rust still makes us check for the outcome. It's just more idiomatic this way.

(and you don't strictly need to unwrap, pattern matching (e.g. let Some(geom) = feat.geometry() else { continue; }) is usually enough)

@phayes

phayes commented Dec 5, 2022

Copy link
Copy Markdown
Contributor Author

@metasim, @jdroenner , @ChristianBeilschmidt - do any of you have an opinion on panic vs Option?

If there's consensus, I'll rework this to return an Option

@metasim

metasim commented Dec 5, 2022

Copy link
Copy Markdown
Contributor

@phayes 1000% Option.

@phayes

phayes commented Dec 5, 2022 •

Copy link
Copy Markdown
Contributor Author

Thanks @metasim , @lnicola . I have now reworked this PR so that Feature::geometry returns an Option

@metasim

metasim commented Dec 5, 2022

Copy link
Copy Markdown
Contributor

bors r=lnicola

@metasim
metasim self-requested a review December 5, 2022 18:20
@bors

bors Bot commented Dec 5, 2022

Copy link
Copy Markdown
Contributor

@bors
bors Bot merged commit 284b1b6 into georust:master Dec 5, 2022
@metasim metasim mentioned this pull request May 18, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants