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

Minor Documentation Fixes #2768

Merged
merged 4 commits into from
Aug 7, 2018
Merged

Minor Documentation Fixes #2768

merged 4 commits into from
Aug 7, 2018

Conversation

dpseidel
Copy link
Collaborator

A second round of minor doc fixes throughout stack. Most of the changes are just towards consistently adding () at the end of function names throughout.

Themes documentation has required the most reworking and will be contained in a separate PR.

@batpigandme, any chance you would take a look through this 2nd massive PR of mine??? 🙏

R/geom-point.r Outdated
#' The \emph{bubblechart} is a scatterplot with a third variable mapped to
#' the size of points. There are no special names for scatterplots where
#' another variable is mapped to point shape or colour, however.
#' appropriate. A \emph{bubblechart} is a scatterplot with a third variable
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

_bubblechart_?

R/save.r Outdated
@@ -32,6 +32,7 @@
#' ggsave("mtcars.pdf", width = 4, height = 4)
#' ggsave("mtcars.pdf", width = 20, height = 20, units = "cm")
#'
#' # cleanup unwanted files with base unlink()
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't think unwanted is quite the right word

R/stat-bin.r Outdated
@@ -9,15 +9,15 @@
#' bin width of a time variable is the number of seconds.
#' @param bins Number of bins. Overridden by `binwidth`. Defaults to 30.
#' @param center The center of one of the bins. Note that if center is above or
#' below the range of the data, things will be shifted by an appropriate
#' number of `width`s. To center on integers, for example, use
#' below the range of the data, things will be shifted by the appropriate
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It's supposed to imply that it will be shifted by an integer multiple of widths to lie within the range of the data. I'm not sure how to say that better

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Might be easier to document center and boundary together?

Copy link
Contributor

@batpigandme batpigandme left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Couple comments, but nothing major. Well played! 👏

@@ -2,28 +2,28 @@
#'
#' Visualise the distribution of a single continuous variable by dividing
#' the x axis into bins and counting the number of observations in each bin.
#' Histograms (`geom_histogram`) display the count with bars; frequency
#' polygons (`geom_freqpoly`) display the counts with lines. Frequency
#' Histograms (`geom_histogram()`) display the count with bars; frequency
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You have "Histograms…display the count" and "frequency polygons…display the counts"— I'm not partial to one or the other, I just think they should be the same.

R/geom-smooth.r Outdated
#' `geom_smooth` and `stat_smooth` are effectively aliases: they
#' both use the same arguments. Use `geom_smooth` unless you want to
#' `geom_smooth()` and `stat_smooth()` are effectively aliases: they
#' both use the same arguments. Use `geom_smooth()` unless you want to
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe state this in the positive? Use stat_smooth() if you want to display the results with a non-standard geom.

R/geom-text.r Outdated
@@ -10,16 +10,16 @@
#' space they occupy on the plot is not constant in data units: when you
#' resize a plot, labels stay the same size, but the size of the axes changes.
#'
#' `geom_text` and `geom_label` both add a label for each row in the
#' `geom_text()` and `geom_label()` both add a label for each row in the
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add labels for each row? (Not wed to it one way or another, they both make sense)

R/labels.r Outdated
#'
#' You can also set axis and legend labels in the individual scales (using
#' the first argument, the `name`). I recommend doing that if you're
#' the first argument, the `name`). I recommend doing this if you're
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Not having read through the docs as carefully as you have, I don't know the answer to this, but is use of the first-person voice common? If not, I'd switch to passive.

R/stat-bin.r Outdated
#' `width = 1` and `center = 0`, even if `0` is outside the range
#' of the data. At most one of `center` and `boundary` may be
#' specified.
#' @param boundary A boundary between two bins. As with `center`, things
#' are shifted when `boundary` is outside the range of the data. For
#' example, to center on integers, use `width = 1` and \code{boundary =
#' 0.5}, even if `0.5` is outside the range of the data. At most one of
#' example, to center on integers, use `width = 1` and `boundary = 0.5`,
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Plus one for this being easier to describe with the center param (though you've made a valiant effort here).

R/stat-bin.r Outdated
#' example, to center on integers, use `width = 1` and \code{boundary =
#' 0.5}, even if `0.5` is outside the range of the data. At most one of
#' example, to center on integers, use `width = 1` and `boundary = 0.5`,
#' even if `0.5` is outside the range of the data. At most one of
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

"At most one of…" just sounds off to me. Do you mean one each?

R/stat-smooth.r Outdated
#' `gam`, `loess`, `MASS::rlm`.
#' @param method Smoothing method (function) to use, accepts either a character vector,
#' e.g. `"auto"`, `"lm"`, `"glm"`, `"gam"`, `"loess"` or a function, e.g.
#' `MASS::rlm` or `mgcv::gam`, `lm`, or `loess`.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Do you mean to have lm and loess in both places? If yes, is that because they are base functions? If so, I'd match them with MASS::rlm by prefixing with base::

R/stat-smooth.r Outdated
@@ -8,7 +9,7 @@
#' `loess` gives a better appearance, but is \eqn{O(N^{2})}{O(N^2)} in memory,
#' so does not work for larger datasets.
#'
#' If you have fewer than 1,000 observations but want to use the same `gam`
#' If you have fewer than 1,000 observations but want to use the same `gam()`
#' model that `method = "auto"` would use then set
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Comma between "use" and "then"

@dpseidel
Copy link
Collaborator Author

So before I merge: their was casual discussion on #2740, about tidy styling paired with a PR like this, since 79 files are already being changed so file commit histories will already be skewed. Is that something that should be considered in this same PR? or done in a separate tidying PR and merged at the same time? If it's something we want to have done, I'm happy to review a separate PR or do it myself here. If not, I'll just merge this in and discussion can continue in 2740. Thanks everyone for the feedback! I know these big PRs are tedious.

Merge remote-tracking branch 'upstream/master' into docs2.0

# Conflicts:
#	R/geom-text.r
#	man/geom_text.Rd
@hadley
Copy link
Member

hadley commented Aug 7, 2018

I don't want to think about style tidying right now, so lets shelve and think about in the future.

@dpseidel dpseidel merged commit 344bfea into tidyverse:master Aug 7, 2018
@dpseidel dpseidel deleted the docs2.0 branch August 8, 2018 17:13
@lock
Copy link

lock bot commented Feb 4, 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 4, 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