-
Notifications
You must be signed in to change notification settings - Fork 96
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
Unified raster creation options over CplStringList
.
#519
Conversation
/// [`Driver::create_with_band_type_with_options`](crate::Driver::create_with_band_type_with_options`). | ||
/// | ||
/// See `papszOptions` in [GDAL's `Create(...)` API documentation](https://gdal.org/api/gdaldriver_cpp.html#_CPPv4N10GDALDriver6CreateEPKciii12GDALDataType12CSLConstList). | ||
pub type RasterCreationOptions = CslStringList; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Not 100% sure if
a) A type alias is a good idea
b) It should be in a separate file
Rationale:
a) If we ever needed to convert it into a newtype (to add creation-specific methods like validate
, it's less breaking. Also, it's easy to get create options and other options confused, so this makes it a bit more clear.
b) Putting a single type in mod.rs
feels unbalanced.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
a) If we ever needed to convert it into a newtype (to add creation-specific methods like validate, it's less breaking. Also, it's easy to get create options and other options confused, so this makes it a bit more clear.
We should probably re-export it from the parent module, so not breaking.
b) Putting a single type in mod.rs feels unbalanced.
There might be more 🙂.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We should probably re-export it from the parent module, so not breaking.
It is:
Question: Should we also update Lines 7 to 12 in 1b8ba56
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, but I secretly wish we just took &[&str]
and built those under the scenes. 😅
@@ -578,23 +578,12 @@ impl<'a> RasterBand<'a> { | |||
/// ```rust, no_run | |||
/// # fn main() -> gdal::errors::Result<()> { | |||
/// use gdal::DriverManager; | |||
/// use gdal::raster::{Buffer, RasterCreationOption}; | |||
/// use gdal::raster::{Buffer, RasterCreationOptions }; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nit:
/// use gdal::raster::{Buffer, RasterCreationOptions };
CHANGES.md
if knowledge of this change could be valuable to users.This:
is now this: