Make title, subtitle, caption and tag as labs()'s named arguments #2669

yutannihilation · 2018-05-29T00:01:46Z

As suggested in #2663 (comment), making them as the named arguments let us organize the document easier.

Note that one difficulty I found is that NULL should be distinguished from missing; NULL indicates the user's intent to remove the label. For example, the code below doesn't show the title:

ggplot() +
  labs(title = "foo") +
  labs(title = NULL)

So, I chose to use missing(). But, if there is some better way to do this with rlang, please point it out.

Besides, I slightly suspect ggtitle() may not do the right thing; the code below doesn't show the subtitle since ggtitle() sets subtitle to NULL. Is this valid for the user's intent?

ggplot() +
  labs(subtitle = "bar") +
  ggtitle(title = "foo")

If this should also be changed, I'll incorporate in this PR as well.

ptoche · 2018-05-30T07:56:25Z

See also: #2446

yutannihilation · 2018-05-30T08:09:01Z

Oh, thanks. If that is intended, I'm ok with it.

ptoche · 2018-05-30T08:49:55Z

I don't like it either.

hadley · 2018-05-30T13:40:49Z

I think we should make labs() and ggtitle() work in the same way, i.e. using missing arguments and don't override without explicit NULL.

hadley · 2018-05-30T13:42:21Z

R/labels.r

  args <- rename_aes(args)
+
+  # Since NULL should be preserved, we should wrap args with list()
+  if (!missing(title)) args["title"] <- list(title)


@lionel- can you think of a more elegant way to write this?

Perhaps remove the named arguments and let users pass them through ...?

That's what we did previously 😄 We're adding them as named params here for better docs + autocomplete.

Hmm I can't think of anything better.

Thanks, then I leave this as is :)

yutannihilation · 2018-05-30T23:15:34Z

I think we should make labs() and ggtitle() work in the same way

I'm OK with either. But, as @batpigandme pointed on #2446, this behavior of ggtitle() is documented. So, I rather hesitate to make a tiny breaking change on this, though it seems negligible.

Maybe is it better to leave this as is and encourage the users to use labs() (and possibly to soft-deprecate ggtitle()?).

clauswilke · 2018-05-30T23:48:49Z

I disagree with @batpigandme's reading of the documentation. The current documentation says "Leave NULL for no subtitle." That's a far cry from "If you leave the subtitle as NULL then your pre-existing subtitle will be deleted."

I'm with @hadley. Don't override without explicit NULL. To be absolutely clear about how things work, we could add the following to the description:

If a plot already has a title, subtitle, caption, etc., and you want to remove it, you can do so by setting the respective argument to NULL. For example, if plot p has a subtitle, then p + labs(subtitle = NULL) will remove the subtitle from the plot.

I also think the description for label is confusing. It currently says "The text for the axis, plot title or caption below the plot", but there is no function where label sets the caption. How about:

The title of the respective axis (for xlab() or ylab()) or of the plot (for ggtitle()).

yutannihilation · 2018-05-31T00:17:37Z

Thanks! Sounds reasonable to me.

lionel- · 2018-05-31T07:24:06Z

For programmability it might be better to use another sentinel value like "" or NA. For documentation / discoverability it seems using missingness is better.

yutannihilation · 2018-05-31T07:53:53Z

I see. There's a trade off. For here, I want to choose missingness for better documentation.

yutannihilation · 2018-05-31T08:08:49Z

Sorry, I'm not sure what is the good way to pass missingness to other function. I'll try, but any advice will be appreciated!

yutannihilation · 2018-05-31T08:29:21Z

Done. Thanks @clauswilke for the sentences.

I'm not sure what is the good way to pass missingness to other function

In this case, I could take the advantage of the fact that labs() can take a list as its first argument.

lionel- · 2018-05-31T08:54:08Z

To pass missingness programmatically you need some meta-programming:

with_qq <- function(expr) {
  expr <- enexpr(expr)
  eval_bare(expr, caller_env())
}

f <- function(x) {
  missing(x)
}
wrapper <- function(x) {
  with_qq(f(!!maybe_missing(x)))
}

wrapper()
#> [1] TRUE

wrapper(1)
#> [1] FALSE

yutannihilation · 2018-05-31T12:42:45Z

Thanks! maybe_missing() seems interesting.

batpigandme · 2018-05-31T12:52:25Z

I disagree with @batpigandme's reading of the documentation. The current documentation says "Leave NULL for no subtitle." That's a far cry from "If you leave the subtitle as NULL then your pre-existing subtitle will be deleted."

^^ @clauswilke this makes sense to me. 👍

hadley · 2018-05-31T14:03:26Z

There's also waiver() which is used elsewhere in ggplot2 for a similar task.

thomasp85 · 2018-05-31T14:30:30Z

waiver as default is def the way to go IMO

yutannihilation · 2018-06-01T09:14:19Z

Ah, I forgot about waiver(). Thanks!

yutannihilation · 2018-06-01T09:35:29Z

R/labels.r

+#' p + labs(title = "title") + labs(title = NULL)
+labs <- function(..., title = waiver(), subtitle = waiver(), caption = waiver(), tag = waiver()) {
+  args <- list(..., title = title, subtitle = subtitle, caption = caption, tag = tag)
+  if (length(args) > 0 && is.list(args[[1]]) && !is.waive(args[[1]])) args <- args[[1]]


By the way, why should labs() accept a list as its first argument?
I couldn't find this is documented or used elsewhere.

Now that ggplot2 uses tidy eval, I think we should remove this, and in the previous line use rlang::list2() instead of list(). That way you can use !!! if you do already have a list of labels.

thomasp85 · 2018-06-01T09:52:19Z

My guess is to avoid the do.call need if you programmatically collect the labels in a list prior to plot construction

yutannihilation · 2018-06-01T10:03:33Z

Thanks, if this is useful for some people, I leave this as is.

hadley · 2018-06-01T12:39:37Z

I’d be happy to remove that feature if it doesn’t break existing code (but it probably will)

yutannihilation · 2018-06-01T12:57:51Z

OK. I just hoped no one knows this feature because it doesn't used internally in ggplot2 and no documents seems to advertise it. But, I agree with your concern.

Then, I think this is ready now.

hadley

Sorry this has taken so long, but I think we've finally iterated to the correct approach 😄

hadley · 2018-07-08T22:45:40Z

R/labels.r

+#' p + labs(title = "title") + labs(title = NULL)
+labs <- function(..., title = waiver(), subtitle = waiver(), caption = waiver(), tag = waiver()) {
+  args <- list(..., title = title, subtitle = subtitle, caption = caption, tag = tag)
+  if (length(args) > 0 && is.list(args[[1]]) && !is.waive(args[[1]])) args <- args[[1]]


Now that ggplot2 uses tidy eval, I think we should remove this, and in the previous line use rlang::list2() instead of list(). That way you can use !!! if you do already have a list of labels.

hadley · 2018-07-08T22:45:58Z

tests/testthat/test-labels.r

+
+    # ggtitle works in the same way as labs()
+    expect_identical(ggtitle("my title")$title, "my title")
+    expect_identical(ggtitle("my title",


Can you please use tidyverse style here? http://style.tidyverse.org/syntax.html#long-lines

yutannihilation · 2018-07-09T11:04:56Z

Sorry, I'm yet to figure out how to squash the duplicated arguments properly... Will try tomorrow or after.

yutannihilation · 2018-07-15T02:45:40Z

@hadley
I think I've implemented what you suggested. But..., I'm wondering if this is OK in terms of the compatibility with the existing codes, as you said:

I’d be happy to remove that feature if it doesn’t break existing code (but it probably will)

Do I get something wrong?

library(ggplot2)

labs(title = "test", tag = "A")
#> $title
#> [1] "test"
#> 
#> $tag
#> [1] "A"
#> 
#> attr(,"class")
#> [1] "labels"

# NULL is preserved
labs(title = NULL)
#> $title
#> NULL
#> 
#> attr(,"class")
#> [1] "labels"

# !!! can be used in labs()
labs(!!!list(title = "test"))
#> $title
#> [1] "test"
#> 
#> attr(,"class")
#> [1] "labels"

# If the argument is duplicated, the first one will be used
labs(!!!list(title = "test"), title = "TEST")
#> $title
#> [1] "test"
#> 
#> attr(,"class")
#> [1] "labels"

Created on 2018-07-15 by the reprex package (v0.2.0).

hadley · 2018-07-23T12:47:01Z

I think this looks good. Can you please now prepare a news bullet?

yutannihilation · 2018-07-25T05:22:13Z

Thanks, added.

yutannihilation · 2018-08-19T10:42:23Z

I resolved the conflict. Could someone review the news bullet?

clauswilke · 2018-08-19T20:49:27Z

@batpigandme Do we have a tidyverse rule for the Oxford comma? I would probably write

`labs()` now has named arguments `title`, `subtitle`, `caption`, and `tag`.

and delete "explicitly".

@yutannihilation Note that any further commits will cause a broken build on Travis until #2838 is resolved. You may want to wait until that time and then rebase.

batpigandme · 2018-08-19T21:20:38Z

@clauswilke we are pro-Oxford-comma!

yutannihilation · 2018-08-22T11:02:25Z

Added the Oxford-comma, and confirmed the checks pass after merging upstream/master.

yutannihilation · 2018-08-24T14:14:53Z

Resolved a conflict.

yutannihilation · 2018-08-24T14:40:49Z

@hadley @clauswilke
If the news bullet seems good to you, could you approve and merge this? I'm sorry to have make this discussion unnecessarily complicated and long...

yutannihilation · 2018-08-24T23:26:42Z

Thanks!

lock · 2019-02-21T00:01:59Z

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/

yutannihilation added 2 commits May 29, 2018 08:47

add tests for labs()

aa6a882

make label params as the formal arguments of labs()

cbfd97f

hadley reviewed May 30, 2018

View reviewed changes

yutannihilation added 2 commits May 31, 2018 17:16

respect missingness in ggtitle()

83a51ed

improve description about labs()

4a1115f

yutannihilation added 2 commits June 1, 2018 18:29

use waive() to represent missingness in labs()

0ff44f4

fix a typo

cc14ce4

yutannihilation commented Jun 1, 2018

View reviewed changes

hadley reviewed Jul 8, 2018

View reviewed changes

apply tidyverse style

0c2008f

Enable !!! in labs()

755ed17

yutannihilation added 2 commits July 25, 2018 14:17

Merge remote-tracking branch 'upstream/master' into labs-named-args

7e181ad

Add a news bullet

ccf014f

Merge remote-tracking branch 'upstream/master' into labs-named-args

65eda76

Add the Oxford comma

138474d

This comment has been minimized.

Sign in to view

Merge remote-tracking branch 'upstream/master' into labs-named-args

bff6b5e

clauswilke mentioned this pull request Aug 23, 2018

allow geom_sf to accept line parameters (#2826) #2847

Merged

Merge remote-tracking branch 'upstream/master' into labs-named-args

273cb5f

clauswilke approved these changes Aug 24, 2018

View reviewed changes

clauswilke merged commit 6500a30 into tidyverse:master Aug 24, 2018

yutannihilation deleted the labs-named-args branch August 24, 2018 23:26

mikejiang mentioned this pull request Oct 29, 2018

test failure due to the API change from labs() in ggplot3.1.0 RGLab/ggcyto#41

Closed

lock bot locked and limited conversation to collaborators Feb 21, 2019

Make title, subtitle, caption and tag as labs()'s named arguments #2669

Make title, subtitle, caption and tag as labs()'s named arguments #2669

Conversation

yutannihilation commented May 29, 2018 • edited Loading

ptoche commented May 30, 2018 • edited Loading

yutannihilation commented May 30, 2018

ptoche commented May 30, 2018

hadley commented May 30, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yutannihilation commented May 30, 2018

clauswilke commented May 30, 2018 • edited Loading

yutannihilation commented May 31, 2018

lionel- commented May 31, 2018

yutannihilation commented May 31, 2018

yutannihilation commented May 31, 2018

yutannihilation commented May 31, 2018

lionel- commented May 31, 2018

yutannihilation commented May 31, 2018

batpigandme commented May 31, 2018

hadley commented May 31, 2018

thomasp85 commented May 31, 2018

yutannihilation commented Jun 1, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

thomasp85 commented Jun 1, 2018

yutannihilation commented Jun 1, 2018

hadley commented Jun 1, 2018

yutannihilation commented Jun 1, 2018

hadley left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

yutannihilation commented Jul 9, 2018

yutannihilation commented Jul 15, 2018

hadley commented Jul 23, 2018

yutannihilation commented Jul 25, 2018

yutannihilation commented Aug 19, 2018

clauswilke commented Aug 19, 2018

batpigandme commented Aug 19, 2018

This comment has been minimized.

yutannihilation commented Aug 22, 2018

yutannihilation commented Aug 24, 2018

yutannihilation commented Aug 24, 2018

yutannihilation commented Aug 24, 2018

lock bot commented Feb 21, 2019

yutannihilation commented May 29, 2018 •

edited

Loading

ptoche commented May 30, 2018 •

edited

Loading

clauswilke commented May 30, 2018 •

edited

Loading