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

Add feature_count{,_force} and implement Iterator::size_hint #108

Merged
merged 1 commit into from
Oct 29, 2020

Conversation

ttencate
Copy link
Contributor

Nice for progress bars and just generally good to have proper size_hints if cheap to compute.

Copy link
Member

@michaelkirk michaelkirk left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not a gdal crate maintainer, just a fan of the gdal crate, so feel free to ignore my review.

Looks like a useful feature though!

@@ -112,6 +112,19 @@ impl<'a> Layer<'a> {
Ok(())
}

pub fn feature_count(&self) -> Option<u64> {
let rv = unsafe { gdal_sys::OGR_L_GetFeatureCount(self.c_layer, 0) };
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'm not a big gdal user - and didn't know what "force" or negative numbers meant in this context. It might be worth surfacing some of the documentation, otherwise users of this crate have to defer to the rust source to see which upstream method we're calling and then look up the upstream docs for that method.

from https://gdal.org/api/vector_c_api.html#_CPPv421OGR_L_GetFeatureCount9OGRLayerHi

If bForce is FALSE, and it would be expensive to establish the feature count a value of -1 may be returned indicating that the count isn’t know. If bForce is TRUE some implementations will actually scan the entire layer once to count objects.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

yes, some documentation would be great

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I was just following the example of the surrounding code ;)

On second thought, I'm not happy with the API that I wrote here. It's better than the existing get_extent(&self, force: bool), but I think feature_count and feature_count_force should be named try_feature_count and feature_count, respectively. This is consistent with TryInto and Into from the standard library.

I'll send a separate API-breaking PR to use that pattern for get_extent too. Ah, this 0.x version number is so liberating!

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

i think it is a good idea to split it into two methods. I guess we should wait for the get_extent change before we release a new version.

@ttencate ttencate force-pushed the feature/feature_count branch 2 times, most recently from 4d03ff9 to ac39424 Compare October 28, 2020 09:09
@ttencate ttencate force-pushed the feature/feature_count branch from ac39424 to df4e3de Compare October 28, 2020 09:13
@jdroenner jdroenner merged commit 02b54b7 into georust:master Oct 29, 2020
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