Skip to content

Fix coord_sf() regression introduced in draw_axis rewrite #3361

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

Merged
merged 3 commits into from
Jun 18, 2019

Conversation

paleolimbot
Copy link
Member

In the draw_axis() rewrite (#3349), I neglected a piece of code that allowed list()s of language objects to be passed as labels. This is only used in coord_sf() to draw degree signs next to labels, and because the coord_sf() tests are all skipped, I didn't notice that I had missed this piece of code. This PR fixes the regression so that coord_sf() plots have degree signs in axis labels once more.

@clauswilke
Copy link
Member

I've been thinking that the decision to skip the sf tests may not be a good one. I realize the problems created by tests whose outcome depends on the version of other packages, but it may be better to update the reference images to those tests from time to time than to risk introducing major regressions.

@paleolimbot
Copy link
Member Author

I agree! The issue is #2684.

@paleolimbot paleolimbot requested a review from hadley June 12, 2019 18:44
@clauswilke
Copy link
Member

It seems like 2 of the 3 items I listed in that issue have been resolved already. Should we turn tests for sf back on and see what happens? @hadley?

@hadley
Copy link
Member

hadley commented Jun 12, 2019

Maybe we should just skip them on CRAN? That way we'll still see on travis and locally, but changes to sf or other stream packages won't force a ggplot2 release.

@clauswilke
Copy link
Member

All visual tests are already skipped on CRAN. That's what I meant when I said 2 of the 3 items I listed in the issue have been resolved: We skip visual tests on CRAN, and on Travis we only run visual tests for the currently released R version.

@hadley
Copy link
Member

hadley commented Jun 12, 2019

Ok got it, then it's definitely worth re-enabling.

@paleolimbot
Copy link
Member Author

I think the instability was because we were reading the North Carolina dataset from a shapefile and/or gpkg. I changed this to be a polygon from one county constructed manually, which results in the dopplegangers passing on the CIs and my computer.

@paleolimbot paleolimbot merged commit cb41aa0 into tidyverse:master Jun 18, 2019
@paleolimbot paleolimbot deleted the fix-coord-sf-regression branch June 18, 2019 12:32
@lock
Copy link

lock bot commented Dec 15, 2019

This old issue has been automatically locked. If you believe you have found a related problem, please file a new issue (with reprex) and link to this issue. https://reprex.tidyverse.org/

@lock lock bot locked and limited conversation to collaborators Dec 15, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants