Add raster creation options #193
Conversation
|
It was not a bug in |
src/raster/tests.rs
Outdated
| #[test] | ||
| fn test_create_with_band_type_with_options() { | ||
| let driver = Driver::get("GTiff").unwrap(); | ||
| let options = vec
There was a problem hiding this comment.
| let options = vec
Co-authored-by: Laurențiu Nicola <[email protected]>
Co-authored-by: Laurențiu Nicola <[email protected]>
…ardtke/gdal into add_raster_creation_options
|
Thanks for reviewing my changes and your suggestions to improve them, I guess I still have a lot to learn! Sorry I made a bit of a mess incorporating your suggestions! I didn't see the "commit suggestion" button at first (we use gitlab at work and the workflow is a bit different), I'll be more careful next time. |
|
Yeah, don't worry about the commit attribution (I'd squash these anyway if it was my PR), and sorry for taking so long to review. Tests seem to fail now because of some extra bit of whitespace or something like that. Unfortunately, I can't merge it (well, I can, but we use bors instead of merging from the GitHub UI), so someone else must take a look. |
|
Just testing: bors d+ Oh, maybe it isn't set up here? |
|
🔒 Permission denied Existing reviewers: click here to make lnicola a reviewer |
|
yes, it seems to be an extra space in the test. I'll fix it now. |
|
PS: you can add your (presumably) other email address to your GitHub account so that you don't show as two different people. |
|
Can you add a link to the PR in the changelog? |
|
r? @jdroenner |
|
So if you want to look into a builder API in the meanwhile, it would be awesome :-). |
|
Sure, I can open another issue and we discuss there. |
| @@ -89,7 +120,7 @@ impl Driver { | |||
| size_y as c_int, | |||
| bands as c_int, | |||
| T::gdal_type(), | |||
| null_mut(), | |||
| options_c as *mut *mut i8, | |||
There was a problem hiding this comment.
shouldn't this be a *mut * mut char?
There was a problem hiding this comment.
You mean c_char?
Seen as char** from C and const char* const* from C++
There was a problem hiding this comment.
yes, options_c as *mut *mut i8 is correct. I'm not sure how it ended up being null_mut() again, sorry.
There was a problem hiding this comment.
It's not null_mut(), it's options_c as *mut *mut i8, jdroenner is asking if it should be options_c as *mut *mut c_char.
There was a problem hiding this comment.
oh, sorry! my bad. Blame Friday night.
There was a problem hiding this comment.
gdal_sys::GDALCreate expects *mut *mut i8.
There was a problem hiding this comment.
Yeah, it's defined as char** in the GDAL headers. We might be able to improve the bindings, but I think we shouldn't block merging this.
There was a problem hiding this comment.
Any thoughts @jdroenner? Do the bindings need to be updated before merging this? I think I don't have enough knowledge to do that.
There was a problem hiding this comment.
no its fine. i just assumed it to be a char in the bindings.
CHANGES.mdif knowledge of this change could be valuable to users.Please review the additions I made (related to #190 ).
Also, I think there may be a bug in
dataset.metadata_item();which makes the testtest_create_with_band_type_with_options()fail, since I can see the COMPRESSION field in the image structure metadata when usinggdalinfo. Or am I missing something?gdalinfo output:
test error: