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

Unified raster creation options over CplStringList. #519

Merged
merged 1 commit into from
Feb 1, 2024

Conversation

metasim
Copy link
Contributor

@metasim metasim commented Jan 31, 2024

  • 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.

This:

let options = [
    RasterCreationOption {
        key: "TILED",
        value: "YES",
    },
    RasterCreationOption {
        key: "BLOCKXSIZE",
        value: "16",
    },
    RasterCreationOption {
        key: "BLOCKYSIZE",
        value: "16",
    },
];

is now this:

let options = RasterCreationOptions::from_iter([
   "TILED=YES", "BLOCKXSIZE=16", "BLOCKYSIZE=16"
]);

/// [`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;
Copy link
Contributor Author

@metasim metasim Jan 31, 2024

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.

Copy link
Member

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 🙂.

Copy link
Contributor Author

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:

https://github.com/georust/gdal/pull/519/files#diff-eb172c9758a6ca0ee77f152d6a8c864de3a68ea11439bb26fa33ea60d3490155R78

@metasim metasim requested a review from lnicola January 31, 2024 21:42
@metasim
Copy link
Contributor Author

metasim commented Jan 31, 2024

Question: Should we also update DatasetOptions::open_options to do something similar?

gdal/src/options.rs

Lines 7 to 12 in 1b8ba56

pub struct DatasetOptions<'a> {
pub open_flags: GdalOpenFlags,
pub allowed_drivers: Option<&'a [&'a str]>,
pub open_options: Option<&'a [&'a str]>,
pub sibling_files: Option<&'a [&'a str]>,
}

Copy link
Member

@lnicola lnicola left a 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. 😅

@metasim metasim merged commit 66d3485 into georust:master Feb 1, 2024
9 checks passed
@metasim metasim deleted the feature/create-options branch February 1, 2024 19:21
@@ -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 };
Copy link
Member

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 };

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.

None yet

2 participants