Skip to content

Duplicated aesthetics on some documentations of Geom/Stat #3618

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

Closed
yutannihilation opened this issue Nov 11, 2019 · 9 comments · Fixed by #3657
Closed

Duplicated aesthetics on some documentations of Geom/Stat #3618

yutannihilation opened this issue Nov 11, 2019 · 9 comments · Fixed by #3657

Comments

@yutannihilation
Copy link
Member

Probably due to the change of the specification of required_aes (#3506), some documentations of Geom/Stat now show both "x or y" and, "x" and "y" separately on their Aesthetics section. This seems a bit confusing, so it's nice if we can fix this.

For example, https://ggplot2.tidyverse.org/dev/reference/geom_linerange.html#aesthetics:

screenshot of `geom_linerange.html`

@carywreams
Copy link
Contributor

carywreams commented Dec 5, 2019

Is this a matter of modifying R/utilities-help.r to remove any parameter found in x$requires_aes from x$aesthetics() prior to, or in conjunction with, the union() operation ?

from

rd_aesthetics_item <- function(x) {
  req <- x$required_aes
  req <- sub("|", "} \\emph{or} \\code{", req, fixed = TRUE)
  all <- union(req, sort(x$aesthetics()))

  ifelse(all %in% req,
    paste0("\\strong{\\code{", all, "}}"),
    paste0("\\code{", all, "}")
  )
}

to

rd_aesthetics_item <- function(x) {
  req <- x$required_aes
  req <- sub("|", "} \\emph{or} \\code{", req, fixed = TRUE)
  req_aes <- unlist(strsplit(x$required_aes, "|", fixed = TRUE))
  optional_aes <- setdiff(x$aesthetics(),req_aes)
  all <- union(req, sort(optional_aes))

  ifelse(all %in% req,
    paste0("\\strong{\\code{", all, "}}"),
    paste0("\\code{", all, "}")
  )
}

If I'm on the right track, let me know and I'll start figuring out how to do this.

@carywreams
Copy link
Contributor

carywreams commented Dec 5, 2019

Seems unlist(strsplit(self$required_aes, "|", fixed = TRUE)) is used already to pull apart the "|" specs. So perhaps my previous question is answered (have edited above comment to include this approach).

I found that technique in R/stat-.r::aesthetics function definition, which appears to be where the individual required parameters are added into consideration (and thus duplicated for documentation) in R/utilities-help.r::rd_aesthetics_item(), as part of the union() call (see above comment).

While I'm tempted to suggest correcting it in R/stat-.r::aesthetics, some customers of that function may continue to expect required parameters (specified as "x|y" in required aesthetics) to appear as individual items when retrieving the list of aesthetics.

Changing the behavior in R/utilities-help.r::rd_aesthetics_item() to remove the duplicates may only affect documentation.

@carywreams
Copy link
Contributor

Promising (at least on the documentation front)

diff --git a/man/geom_linerange.Rd b/man/geom_linerange.Rd
index 16fed3db..a0e073d1 100644
--- a/man/geom_linerange.Rd
+++ b/man/geom_linerange.Rd
@@ -134,12 +134,6 @@ This geom treats each axis differently and, thus, can thus have two orientations
 \item \code{group}
 \item \code{linetype}
 \item \code{size}
-\item \code{x}
-\item \code{xmax}
-\item \code{xmin}
-\item \code{y}
-\item \code{ymax}
-\item \code{ymin}
 }
 Learn more about setting these aesthetics in \code{vignette("ggplot2-specs")}.
 }

@carywreams
Copy link
Contributor

appears to only affect these files:

#       modified:   geom_bar.Rd
#       modified:   geom_boxplot.Rd
#       modified:   geom_linerange.Rd
#       modified:   geom_ribbon.Rd

@carywreams
Copy link
Contributor

testthat runs produce identical results on master branch and the branch with my candidate mod.

the pull request for #3506 looks to have had some build issues as well. Should I be concerned ?
As the results are the same, think I'm going to wrap this and submit a PR; will deal with any additional issues there.

@carywreams
Copy link
Contributor

looking for a little help with automated code coverage checks in pr #3657

@clauswilke
Copy link
Member

You can ignore them.

@carywreams
Copy link
Contributor

.ignored

Thanks

@lock
Copy link

lock bot commented Jun 24, 2020

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 Jun 24, 2020
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants