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 filetype argument to tar_terra_vect() #21

Merged
merged 12 commits into from
Mar 15, 2024
Merged

Conversation

Aariq
Copy link
Collaborator

@Aariq Aariq commented Mar 13, 2024

I tried to replicate additions to tar_terra_rast() in tar_terra_vect(). I think ESRI Shapfiles need special handling, and potentially other formats won't work. I made the default format GeoJSON simply because it is an included driver and produces a single file.

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

Aariq commented Mar 13, 2024

Not quite working yet. Somewhere I've not defined the object gdal. Can investigate tomorrow

@njtierney
Copy link
Owner

I perhaps wonder if once #19 is merged and this is rebased/merged off of that if it will help resolve some of those changes?

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.

Looks good to me - one small suggestions on if vs ifelse, what do you think?

R/tar-terra-vect.R Outdated Show resolved Hide resolved
Co-authored-by: Nicholas Tierney <njtierney@users.noreply.github.com>
@njtierney
Copy link
Owner

It passes checks! Did you want to merge now?

@Aariq
Copy link
Collaborator Author

Aariq commented Mar 14, 2024

If you'd rather merge #19 first, I can update this PR to use default options for filetype. Or we can merge them both and do that in a separate PR. Doesn't matter to me.

@njtierney
Copy link
Owner

I think I'd prefer to merge #19 first actually - there are a couple of changes that you requested, I'll comment on them in the PR - but maybe these could be addressed in separate issues.

Copy link
Contributor

@brownag brownag left a comment

Choose a reason for hiding this comment

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

Overall looks good to me, just spotted one tiny typo. Thanks for adding the links too!

We will need to create options for vector data to fix the {covr} failure, and should hash out the naming in #25

I spent a little bit looking into the mechanics of {covr} last night... for now I do not have an alternate solution to what was done in #19, so will want to use defaults via the geotargets_option_get() approach for vector data too

R/tar-terra-rast.R Outdated Show resolved Hide resolved
Co-authored-by: Andrew Gene Brown <brown.andrewg@gmail.com>
Merge commit 'b16bea407a74aad2cba1f708d37bd749a5807454'

#Conflicts:
#	R/tar-terra-vect.R
@Aariq Aariq merged commit f24dd74 into master Mar 15, 2024
6 checks passed
@Aariq Aariq deleted the filetype_terra_vect branch March 15, 2024 22:00
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.

Implement "filetype" argument
3 participants