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 geotargets options functions, with defaults for raster GDAL driver and creation options #19

Merged
merged 10 commits into from
Mar 15, 2024

Conversation

brownag
Copy link
Contributor

@brownag brownag commented Mar 13, 2024

Two new draft functions were added for #16 : geotargets_option_get() and geotargets_option_set().

I noticed that the code coverage was failing. There seems to be an issue with modifying the function body for create_format_terra_raster() inside covr::package_coverage(). This PR fixes that.

It appears coverage issue can be resolved by allowing for the default options for GDAL driver (filetype) and creation options (gdal) to be used in the template function, rather than NULL. Though I am still curious what is happening inside the evaluation {covr} does that causes this to fail.

Copy link
Owner

@njtierney njtierney left a comment

Choose a reason for hiding this comment

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

Thanks so much for these changes, @brownag ! This is awesome.

Just a couple of suggestions regarding abstracting out some parts into functions - let me know if you would prefer not to do these?

I also think we will want to import rlang - it's already in targets so we aren't adding a dependency, and I think we could benefit from some rlang functions, such as %||%.

R/geotargets-option.R Outdated Show resolved Hide resolved
R/geotargets-option.R Outdated Show resolved Hide resolved
R/geotargets-option.R Outdated Show resolved Hide resolved
R/tar-terra-rast.R Outdated Show resolved Hide resolved
@njtierney
Copy link
Owner

Also thank you so much for catching that {covr} error - I had just chalked it up to "CI be weird sometimes" and shelved it for later. This is a huge help!

@njtierney njtierney mentioned this pull request Mar 13, 2024
@Aariq Aariq linked an issue Mar 13, 2024 that may be closed by this pull request
Copy link
Collaborator

@Aariq Aariq left a comment

Choose a reason for hiding this comment

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

A few ideas/suggestions and a request to get an error if you ask to set/get an invalid option name.

R/geotargets-option.R Show resolved Hide resolved
R/geotargets-option.R Show resolved Hide resolved
R/geotargets-option.R Show resolved Hide resolved
@njtierney njtierney merged commit d7b2176 into njtierney:master Mar 15, 2024
6 checks passed
@brownag brownag mentioned this pull request Mar 27, 2024
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.

import rlang
3 participants