Implement wrapper for OGR_L_TestCapability #160
Conversation
dmarteau
force-pushed
the
layer_capabilities
branch
from
f8e45aa to
e910ccb
Compare
dmarteau
force-pushed
the
layer_capabilities
branch
from
e910ccb to
6e90843
Compare
| @@ -101,6 +146,13 @@ impl<'a> Layer<'a> { | |||
| _string(rv) | |||
| } | |||
|
|
|||
| pub fn has_capability(&self, capability: LayerCaps::Type) -> Result<bool> { | |||
| let rv = unsafe { | |||
| gdal_sys::OGR_L_TestCapability(self.c_layer, CString::new(capability)?.as_ptr()) == 1 | |||
There was a problem hiding this comment.
When would this fail? If you do go the way of only accepting an enum of input strings, I'd argue you could unwrap here and offer a nicer API to your users: bool instead of Result<bool>.
I think we're better equipped to carefully scrutinize unwrap code centrally in a library like this, and save the attention of downstream users who now will have to ask "will their code explode" whenever they use this method.
Feel free to ignore this suggestion, reasonable minds may disagree. =)
There was a problem hiding this comment.
I'd love to get rid of the Result<>, but the input type is actually a alias to '&'static str' so it does not prevent to use invalid candidate input that may fail the conversion to CString. If everybody think that the intention notice is ok, I will happily go with unwrap.
There was a problem hiding this comment.
I don't think unwrap() in the present implementation is acceptable. I believe what @michaelkirk suggested is to accept a enum as input rather than &str in which case we could even get rid of the &str -> &Cstr conversion. That is a lot of boilerplate (unless there's a nice macro crate), so we could postpone it to a later PR.
There was a problem hiding this comment.
If the input type doesn't forbid passing in something that would explode, I would personally be in favor of keeping the Result as is.
There was a problem hiding this comment.
So, if we could somehow define the consts in mod LayerCaps directly as &'static Cstr, then we can avoid both the extra allocation to convert &str to &Cstr, as well as the Result<>. However, it seems to need some complicated macro: see SO post.
There was a problem hiding this comment.
Oh wow, good find. Looks fun.
There was a problem hiding this comment.
Implemented the enum alternative solution, so one may compare.
There was a problem hiding this comment.
The enum alternative does not avoid the conversion but limit the input to acceptable values so conversion to CStr will not return an error.
There was a problem hiding this comment.
So, should we close that PR in favor of #162 ?
|
i merged the enum version. |
CHANGES.mdif knowledge of this change could be valuable to users.OLCxxx GDALpreprocessor defines toLayerCaps::OLCxxxconstants.OGR_L_TestGetCapabilityasLayer::test_capability(&self, capability: LayerCaps::Type) -> Result<bool>