Add arrow to line params

[JakeRuss]

My vague recollection when I opened #4391, was I thought adding arrow was basically adding it to the list. As you can see from this commit it doesn't work.

I expected the following to work, but no.

library(sf)
library(ggplot2)

coords <- tibble::tibble(lat = c(26.562855, 28.538336), 
                         lon = c(-81.949532, -81.379234)) %>%
  st_as_sf(coords = c("lon", "lat"),  crs = 4326, remove = FALSE) %>%
  st_coordinates() %>%
  st_linestring()

ggplot() + 
  geom_sf(data = coords, size = 2, 
          arrow = arrow(angle = 45, ends = "last", type = "open", length = unit(0.5, "inches")))

I'm not sure I understand enough to complete this task

[clauswilke]

Arrows are not set in gpar(), they are set directly in the grob. See e.g. the documentation to linesGrob(). I suspect that if you add the arrow info here it will work:

ggplot2/R/geom-sf.R

Line 216 in 6d19950

sf::st_as_grob(x$geometry, pch = pch, gp = gp)

[JakeRuss]

Thank you @clauswilke, that does seem to have done the trick. My example above now works.

I guess add a bullet to news. Is there anything else I need to add here?

[clauswilke]

Maybe also add a visual test?

[JakeRuss]

My apologies, but I'm not sure what that means. If you could point me to an example I'd gladly add it.

[clauswilke]

See: https://vdiffr.r-lib.org/

Example:

ggplot2/tests/testthat/test-geom-sf.R

Lines 177 to 204 in 6e3ac81

	test_that("geom_sf_text() and geom_sf_label() draws correctly", {
	skip_if_not_installed("sf")
	if (packageVersion("sf") < "0.5.3") skip("Need sf 0.5.3")
	nc_tiny_coords <- matrix(
	c(-81.473, -81.741, -81.67, -81.345, -81.266, -81.24, -81.473,
	36.234, 36.392, 36.59, 36.573, 36.437, 36.365, 36.234),
	ncol = 2
	)
	nc <- sf::st_as_sf(
	data_frame(
	NAME = "ashe",
	geometry = sf::st_sfc(sf::st_polygon(list(nc_tiny_coords)), crs = 4326)
	)
	)
	# In order to avoid warning, transform to a projected coordinate system
	nc_3857 <- sf::st_transform(nc, 3857)
	expect_doppelganger("Texts for North Carolina",
	ggplot() + geom_sf_text(data = nc_3857, aes(label = NAME))
	)
	expect_doppelganger("Labels for North Carolina",
	ggplot() + geom_sf_label(data = nc_3857, aes(label = NAME))
	)
	})

[JakeRuss]

Greatly appreciated, I will add

[JakeRuss]

@thomasp85 Ready to review at your schedule

[clauswilke]

@JakeRuss It looks like you didn't commit the new reference images for the two visual tests.

[JakeRuss]

@clauswilke Thank you for all of your pointers

[clauswilke]

The tests are failing. Please take a look. Looks like you need to write sf::st_coordinates() instead of just st_coordinates(). There may be other, similar issues once you fix this one.

[clauswilke]

data_frame() is a function defined by the ggplot2 codebase. It should not require a tibble:: prefix. See also the other tests.

[clauswilke]

The reason why some of the tests still fail is because the reference images differ between the version you committed and the version generated in the github action. You may have outdated versions of some libraries. Not clear which ones, but if not vdiffr then possibly sf.

[JakeRuss]

Thanks, I will clean this up again today. I was running the testthat tests locally and it was complaining about data_frame, which I see now was because I didn't have it loaded.

[clauswilke]

Looks good to me.

@thomasp85 Any comments or concerns from your end?

[thomasp85]

Thanks @JakeRuss !