Skip to content

Conversation

brl0
Copy link
Collaborator

@brl0 brl0 commented Aug 3, 2021

No description provided.

@brl0 brl0 changed the title [WIP] Pass storage options, add typing [WIP] Pass storage options, add some typing Aug 3, 2021
Copy link
Member

@jbednar jbednar left a comment

Choose a reason for hiding this comment

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

Looks good to me!

@brl0
Copy link
Collaborator Author

brl0 commented Aug 6, 2021

@jbednar,

First, thanks for your amazingly quick review!

Since your review, I've made some minor changes (as has @iameskild in a companion PR #79). One notable change of behavior is that I added an overwrite option for parquet output like dask uses as a guard against deleting the destination directory. Currently, if one were to write to an existing destination, even something like the root directory of a bucket or filesystem, spatialpandas calls filesystem.rm(path, recursive=True), which seems dangerous.
I wanted to make sure you are comfortable with this before merging, since it is potentially a slight change of behavior for existing users.

Also, once this PR and #79 are merged, I was planning to cut a new release. Let me know if you have any questions, concerns, etc.

Thanks again!

@brl0 brl0 changed the title [WIP] Pass storage options, add some typing Pass storage options, add some typing, minor clean up Aug 6, 2021
@brl0 brl0 marked this pull request as ready for review August 6, 2021 16:54
@jbednar
Copy link
Member

jbednar commented Aug 6, 2021

The speed of my review is a complex function of what I happen to be in the middle of when it appears, plus how tricky the changes are! In this case it all looks good. I agree with the overwrite flag; seems like a very useful addition. And happy to see a new release when it's done!

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