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

Fix sf c() test now that c.sfc() requires identical crs #1817

Merged
merged 2 commits into from Mar 17, 2023

Conversation

DavisVaughan
Copy link
Member

@DavisVaughan DavisVaughan commented Mar 16, 2023

In sf 1.0-10, they fixed r-spatial/sf#1884, which means that sfc vectors now can't be combined with c() if they have different crs values, which seems reasonable to me.

I've done the minimal work to get our c() tests working correctly again, but I don't know too much about how these sf bits work.

I figure we'd merge this then move sf and maybe data.table to Suggests to ensure they are tested

y = st_sfc(st_point(c(0, 0)), precision = 1e-4, crs = 4326)
expect_snapshot(error = TRUE, {
vctrs::vec_c(x, y)
})
Copy link
Member Author

Choose a reason for hiding this comment

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

I can't seem to figure out how to get rstudio to respect the tab width thing for just one file

Copy link
Member

@lionel- lionel- left a comment

Choose a reason for hiding this comment

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

I figure we'd merge this then move sf and maybe data.table to Suggests to ensure they are tested

Alternatively we just add a skip_on_cran() at the top of the files.

Moving data.table to Suggests has little cost though. I'm a bit worried about sf but maybe it's now alright with PPM binaries and improved caching on GHA?

@DavisVaughan
Copy link
Member Author

I guess adding skip_on_cran() and keeping everything else the same means it would pretty much be guaranteed that these tests would only ever run locally on our personal computers?

@DavisVaughan
Copy link
Member Author

DavisVaughan commented Mar 17, 2023

One of the "bigger" benefits to consider for putting them in Suggests is that sf probably would have let us know about this change earlier on through their revdep check process

Although I agree with you about worrying about install time on CI

@lionel-
Copy link
Member

lionel- commented Mar 17, 2023

I guess adding skip_on_cran() and keeping everything else the same means it would pretty much be guaranteed that these tests would only ever run locally on our personal computers?

yes this was the original idea behind removing from Suggests. At the time I didn't expect CRAN machines to still have sf installed in the checks environment.

@DavisVaughan DavisVaughan merged commit fe1f2ee into r-lib:main Mar 17, 2023
12 checks passed
@DavisVaughan DavisVaughan deleted the fix/sf-test branch March 17, 2023 13:52
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.

dplyr::bind_rows binds sf with different crs without error/warning or reprojection
2 participants