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

Finish implementing vctrs methods for sfc vectors #1196

Merged
merged 11 commits into from
Nov 20, 2019

Conversation

lionel-
Copy link
Contributor

@lionel- lionel- commented Nov 19, 2019

  • Restore n_empty, crs, and precision attributes.

  • Add common type implementation for sfc objects. This finds the common class based on heuristics found in c.sfc().

  • Added a new vctrs help topic, so that argument names can be documented separately from the tidyverse ones.

@lionel- lionel- force-pushed the fix-sfc branch 4 times, most recently from 057e5ba to 5466bef Compare November 19, 2019 20:56
@edzer edzer mentioned this pull request Nov 19, 2019
@lionel-
Copy link
Contributor Author

lionel- commented Nov 20, 2019

Should we move the vctrs methods into their own file vctrs.R or perhaps tidyverse-vctrs.R?

I'm now looking into other data types, it might make the source more easily browsable to keep the vctrs methods separate as we might end up with a non-trivial amount of those.

@edzer
Copy link
Member

edzer commented Nov 20, 2019

Yes, good idea!

@lionel-
Copy link
Contributor Author

lionel- commented Nov 20, 2019

hmm, last Travis build failed when it shouldn't have:

  Error: processing vignette 'sf5.Rmd' failed with diagnostics:

   package or namespace load failed for 'tidyr':

    object 'cnd_bullets' not found whilst loading namespace 'vctrs'

This is because dev rlang and dev vctrs are temporarily in incompatible state.

Is it intended that Travis checks against dev rlang and dev vctrs? I was assuming we'd be testing against CRAN versions.

@yutannihilation
Copy link
Contributor

It seems because some r_github_packages is used by default. I guess they were necessary for development at the time when they were added. Are they still needed now?

sf/.travis.yml

Lines 19 to 24 in fe70fea

r_github_packages:
- r-dbi/DBI
- r-dbi/RPostgres
- r-lib/covr
- r-spatial/lwgeom
- r-spatial/stars

edzer added a commit that referenced this pull request Nov 20, 2019
@lionel-
Copy link
Contributor Author

lionel- commented Nov 20, 2019

Following up on #1172 (comment), the CRS and precision attributes are now combined. I took the conservative approach of throwing an error if they are inconsistent. This can be changed to something more appropriate by modifying common_crs() and common_prec().

@edzer
Copy link
Member

edzer commented Nov 20, 2019

Looks good -- thanks so much!

@edzer edzer merged commit 6946fd9 into r-spatial:master Nov 20, 2019
@lionel- lionel- deleted the fix-sfc branch November 20, 2019 12:35
edzer added a commit that referenced this pull request Nov 21, 2019
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.

3 participants