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 Wrapper for GDALVectorTranslate(ogr2ogr) #536

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

zwishing
Copy link

@zwishing zwishing commented May 6, 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 PR add Wrapper for GDALVectorTranslate which is the equivalent of the ogr2ogr (https://gdal.org/programs/ogr2ogr.html) utility(https://gdal.org/api/gdal_utils.html#_CPPv419GDALVectorTranslatePKc12GDALDatasetHiP12GDALDatasetHPK26GDALVectorTranslateOptionsPi).
Also, I changed MultiDimTranslateDestination enum to DatasetDestination

use crate::utils::_path_to_c_string;

/// Path or Dataset to store Vectors or Raster
pub enum DatasetDestination {
Copy link
Member

Choose a reason for hiding this comment

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

Not sure, but maybe DestinationDataset sounds a little better?

}
}
}
}
Copy link
Member

Choose a reason for hiding this comment

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

Nit: missing trailing newline.

@@ -0,0 +1,8 @@
#[cfg(all(major_ge_3, minor_ge_1))]
mod vector_translate;
Copy link
Member

Choose a reason for hiding this comment

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

This is available since 2.1, not 3.1, isn't it?

pub use vector_translate::{
vector_translate,
VectorTranslateOptions
};
Copy link
Member

Choose a reason for hiding this comment

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

Missing trailing newline.

};
use gdal_sys::{GDALVectorTranslate, GDALVectorTranslateOptions, GDALVectorTranslateOptionsFree};
use libc::c_char;
use crate::programs::destination::DatasetDestination;
Copy link
Member

Choose a reason for hiding this comment

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

This should be pub use, otherwise you can't call vector_translate.

/// See [GDALVectorTranslateOptionsNew].
///
/// [GDALVectorTranslateOptionsNew]: https://gdal.org/api/gdal_utils.html#_CPPv429GDALVectorTranslateOptionsNewPPcP35GDALVectorTranslateOptionsForBinary
pub fn new<S:Into<Vec<u8>>,I:IntoIterator<Item=S>>(args:I)->Result<Self>{
Copy link
Member

Choose a reason for hiding this comment

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

Please run cargo fmt.

Copy link
Member

Choose a reason for hiding this comment

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

I know you mirrored multi_dim_translate, but https://docs.rs/gdal/latest/gdal/cpl/struct.CslStringList.html might be nicer to use here, especially since fn new<S:Into<Vec<u8>>,I:IntoIterator<Item=S>>(args:I)->Result<Self> can look a little scary.

Copy link
Author

Choose a reason for hiding this comment

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

It‘s excellent advice to use CslString instead of fn new<S:Into<Vec<u8>>,I:IntoIterator<Item=S>>(args:I)->Result<Self> , and I will make some modifications to this section.

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.

2 participants