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

Rename and delete datasets #226

Merged
merged 3 commits into from
Oct 21, 2021
Merged

Conversation

ChristianBeilschmidt
Copy link
Contributor

@ChristianBeilschmidt ChristianBeilschmidt commented Oct 18, 2021

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

The Driver was missing the methods rename and delete (and more). I've added them together with a test.

I was quite unsure to use &str or &Path as input since we use both in our codebase, in particular, &Path for Dataset::open. This is why I stuck with &Path here. For creating the CString out of the &Path, I've added a method in util to have a common way to do this.

@@ -50,3 +51,8 @@ pub fn _last_null_pointer_err(method_name: &'static str) -> GdalError {
msg: last_err_msg,
}
}

pub fn _path_to_c_string(path: &Path) -> Result<CString> {
let path_str = path.to_string_lossy();
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn 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.

I don't know, I just copied the existing behavior. Is there a better way?

Copy link
Member

Choose a reason for hiding this comment

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

On Unix, yes, on Windows it's complicated.

src/utils.rs Show resolved Hide resolved
@lnicola
Copy link
Member

lnicola commented Oct 19, 2021

Looks like there's a conflict, can you rebase?

@lnicola
Copy link
Member

lnicola commented Oct 19, 2021

Looks good, I'll merge it tomorrow unless someone else has remarks.

@lnicola
Copy link
Member

lnicola commented Oct 21, 2021

* for some definition of "tomorrow"

bors r+

@bors
Copy link
Contributor

bors bot commented Oct 21, 2021

@bors bors bot merged commit 48911dc into georust:master Oct 21, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants