Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Merged
merged 4 commits into from
Dec 5, 2022

Conversation

phayes
Copy link
Contributor

@phayes phayes commented Dec 5, 2022

  • [✓] 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
Copy link
Member

lnicola commented Dec 5, 2022

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
Copy link
Contributor Author

phayes commented Dec 5, 2022

@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
Copy link
Member

lnicola commented Dec 5, 2022

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
Copy link
Contributor Author

phayes commented Dec 5, 2022

@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
Copy link
Contributor

metasim commented Dec 5, 2022

@phayes 1000% Option.

@phayes
Copy link
Contributor Author

phayes commented Dec 5, 2022

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

@metasim
Copy link
Contributor

metasim commented Dec 5, 2022

bors r=lnicola

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

bors bot commented Dec 5, 2022

@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