-
Notifications
You must be signed in to change notification settings - Fork 4
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
Preserve metadata with zip archives #109
Conversation
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.
This looks great! As mentioned in the review, I think it might be useful/worthwhile to consider using enum / character option instead of TRUE/FALSE for preserve_metadata
, as discussed in https://design.tidyverse.org/boolean-strategies.html.
Re
Question for reviewers:
Should terra, zip, and withr still be in Suggests? Now that it's more clear the main utility of geotargets is to work with terra, we might consider moving it (and the dependencies needed for preserve_metadata) into Imports
I agree with you at terra,
zip, and
withrshould be in Imports now, especially since it is more or less the main goal of
geotargetsto work with
terra` - which we didn't know for sure would be the case when we started the project.
Ok, I've got the geotargets option for preserve metadata worked out, if anyone wants to give feedback on naming things. |
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.
Looks awesome!
Just some small changes! Not sure if the switch statement is necessary, but it might be useful, maybe?
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.
Looks good to me, thanks so much, @Aariq !
This closes #58 and supersedes #63 and #106.
Adds a
preserve_metadata
argument totar_terra_rast()
that whenTRUE
switches the write and read functions to write to a tempdir and zip files before moving them topath
and to unzip that archive before reading in withterra::rast()
. This is a blanket solution (I think) that should work for any raster driver. There are notes as to whyvsizip/
wasn't used innotes/
Question for reviewers:
Should
terra
,zip
, andwithr
still be in Suggests? Now that it's more clear the main utility ofgeotargets
is to work withterra
, we might consider moving it (and the dependencies needed forpreserve_metadata
) intoImports