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

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

Merged
merged 14 commits into from Aug 24, 2018

Conversation

@yutannihilation
Copy link
Member

@yutannihilation yutannihilation commented May 29, 2018

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
Copy link

@ptoche ptoche commented May 30, 2018

See also: #2446

@yutannihilation
Copy link
Member Author

@yutannihilation yutannihilation commented May 30, 2018

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

@ptoche
Copy link

@ptoche ptoche commented May 30, 2018

I don't like it either.

@hadley
Copy link
Member

@hadley hadley commented May 30, 2018

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.

R/labels.r Outdated
args <- rename_aes(args)

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

@hadley hadley May 30, 2018

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

Copy link
Member

@lionel- lionel- May 30, 2018

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

Copy link
Member

@hadley hadley May 30, 2018

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

Copy link
Member

@lionel- lionel- May 30, 2018

Hmm I can't think of anything better.

Copy link
Member Author

@yutannihilation yutannihilation May 30, 2018

Thanks, then I leave this as is :)

@yutannihilation
Copy link
Member Author

@yutannihilation yutannihilation commented May 30, 2018

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
Copy link
Member

@clauswilke clauswilke commented May 30, 2018

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
Copy link
Member Author

@yutannihilation yutannihilation commented May 31, 2018

Thanks! Sounds reasonable to me.

@lionel-
Copy link
Member

@lionel- lionel- commented May 31, 2018

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

@yutannihilation
Copy link
Member Author

@yutannihilation yutannihilation commented May 31, 2018

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

@yutannihilation
Copy link
Member Author

@yutannihilation yutannihilation commented May 31, 2018

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
Copy link
Member Author

@yutannihilation yutannihilation commented May 31, 2018

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-
Copy link
Member

@lionel- lionel- commented May 31, 2018

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
Copy link
Member Author

@yutannihilation yutannihilation commented May 31, 2018

Thanks! maybe_missing() seems interesting.

@batpigandme
Copy link
Member

@batpigandme batpigandme commented May 31, 2018

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
Copy link
Member

@hadley hadley commented May 31, 2018

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

@thomasp85
Copy link
Member

@thomasp85 thomasp85 commented May 31, 2018

waiver as default is def the way to go IMO

@yutannihilation
Copy link
Member Author

@yutannihilation yutannihilation commented Jun 1, 2018

Ah, I forgot about waiver(). Thanks!

R/labels.r Outdated
#' 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]]
Copy link
Member Author

@yutannihilation yutannihilation Jun 1, 2018

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

Copy link
Member

@hadley hadley Jul 8, 2018

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
Copy link
Member

@thomasp85 thomasp85 commented Jun 1, 2018

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

@yutannihilation
Copy link
Member Author

@yutannihilation yutannihilation commented Jun 1, 2018

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

@hadley
Copy link
Member

@hadley hadley commented Jun 1, 2018

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

@yutannihilation
Copy link
Member Author

@yutannihilation yutannihilation commented Jun 1, 2018

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.

Copy link
Member

@hadley hadley left a comment

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

R/labels.r Outdated
#' 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]]
Copy link
Member

@hadley hadley Jul 8, 2018

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.


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

@hadley hadley Jul 8, 2018

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

@yutannihilation
Copy link
Member Author

@yutannihilation yutannihilation commented Jul 9, 2018

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

@yutannihilation
Copy link
Member Author

@yutannihilation yutannihilation commented Jul 15, 2018

@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
Copy link
Member

@hadley hadley commented Jul 23, 2018

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

@yutannihilation
Copy link
Member Author

@yutannihilation yutannihilation commented Jul 25, 2018

Thanks, added.

@yutannihilation
Copy link
Member Author

@yutannihilation yutannihilation commented Aug 19, 2018

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

@clauswilke
Copy link
Member

@clauswilke clauswilke commented Aug 19, 2018

@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
Copy link
Member

@batpigandme batpigandme commented Aug 19, 2018

@clauswilke we are pro-Oxford-comma!

@smouksassi

This comment was marked as off-topic.

@yutannihilation
Copy link
Member Author

@yutannihilation yutannihilation commented Aug 22, 2018

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

@yutannihilation
Copy link
Member Author

@yutannihilation yutannihilation commented Aug 24, 2018

Resolved a conflict.

@yutannihilation
Copy link
Member Author

@yutannihilation yutannihilation commented Aug 24, 2018

@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...

@clauswilke clauswilke merged commit 6500a30 into tidyverse:master Aug 24, 2018
4 checks passed
@yutannihilation
Copy link
Member Author

@yutannihilation yutannihilation commented Aug 24, 2018

Thanks!

@lock
Copy link

@lock lock bot commented Feb 21, 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 Feb 21, 2019
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

8 participants