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

Improve the docs #509

Merged
merged 36 commits into from Oct 24, 2018

Conversation

Projects
None yet
3 participants
@jennybc
Copy link
Member

jennybc commented Oct 17, 2018

Closes #470 Tibble documentation improvements

Highlights:

  • Created new help topic for the tbl_df class. That received lots of content from the old ?tibble-package and ?tibble().
  • Rewrote ?tibble-package. It's shorter and more high-level. Links to other places.
  • Shortened and refocused ?tibble(). Make this about the tibble() constructor function, not the tbl_df class.
  • Expose and centralize docs around formatting and printing.
  • Gave lst() its own help topic with beefed up examples. Moved lst_() to join other "deprecated" functions (more of a state of mind, no formal deprecations).

The only "todo" from #470 I haven't dealt with is the one about documenting new_tibble(). I would need a quick conversation about that first. This will be handled in #512.

@jennybc jennybc force-pushed the f-470-tibble-docs branch from 3faef59 to 937765c Oct 17, 2018

Show resolved Hide resolved R/tibble.R
Show resolved Hide resolved R/utils-format.r

@jennybc jennybc requested review from krlmlr and hadley Oct 21, 2018

@hadley
Copy link
Member

hadley left a comment

Overall this is looking great, but I have lots of thoughts 😄

NEWS.md Outdated
@@ -14,7 +14,7 @@ The `tibble()` and `as_tibble()` functions, and the low-level `new_tibble()` con

- Calling `as_tibble()` on a vector now returns a one-row tibble, for consistency with `as_tibble.list()`. Use `enframe(name = NULL)` for converting a vector to a one-column tibble.

- `data_frame()`, `tibble_()`, `data_frame_()` and `frame_data()` are soft-deprecated, please use `tibble()` or `tribble()` (#111).
- `data_frame()`, `tibble_()`, `data_frame_()`, `lst_()`, and `frame_data()` are soft-deprecated, please use `tibble()` or `tribble()` (#111).

This comment has been minimized.

@hadley

hadley Oct 21, 2018

Member

Might be easier to put lst_ in a separate bullet, since it should have "please use lst()"

#' - matrices
#' - tables
#' - default (coerces to a list)
#' `as_tibble()` is a function, like [tibble()], that is used to make a data

This comment has been minimized.

@hadley

hadley Oct 21, 2018

Member

"is a function" seems a bit redundant. I think maybe you can cut the whole first sentence?

Show resolved Hide resolved R/as_tibble.R Outdated
Show resolved Hide resolved R/as_tibble.R Outdated
Show resolved Hide resolved R/lst.R
Show resolved Hide resolved R/tbl-df.r Outdated
Show resolved Hide resolved R/tbl-df.r Outdated
Show resolved Hide resolved R/tbl-df.r Outdated
Show resolved Hide resolved R/tbl-df.r
Show resolved Hide resolved R/tibble.R Outdated
@jennybc

This comment has been minimized.

Copy link
Member

jennybc commented Oct 21, 2018

I propose we open a new issue re documenting new_tibble(), the only "todo" from #470 that's not addressed here, to make this easier to merge soon. This will be handled in #512.

Show resolved Hide resolved R/tibble.R
@hadley

hadley approved these changes Oct 22, 2018

Show resolved Hide resolved R/utils-format.r Outdated
@hadley

This comment has been minimized.

Copy link
Member

hadley commented Oct 22, 2018

The documentation changes to new_tibble() will be incorporated into #512.

@hadley

hadley approved these changes Oct 22, 2018

@jennybc

This comment has been minimized.

Copy link
Member

jennybc commented Oct 22, 2018

I will merge master in to clear these conflicts. We will squash this anyway, I assume.

Merge branch 'master' into f-470-tibble-docs
# Conflicts:
#	R/repair-names.R
#	man/name-repair.Rd
@jennybc

This comment has been minimized.

Copy link
Member

jennybc commented Oct 22, 2018

@krlmlr I think you can squash-and-merge this now, if you are happy.

@krlmlr
Copy link
Member

krlmlr left a comment

Thanks for updating the docs, I like the new layout a lot. I have mostly minor remarks and suggestions. Would you like to do another pass?

Show resolved Hide resolved R/as_tibble.R Outdated
#'
#' [pkgconfig::set_config()]
#' @seealso [tibble()] constructs a tibble from individual columnns. [enframe()]

This comment has been minimized.

@krlmlr

krlmlr Oct 22, 2018

Member

Typo:

Suggested change Beta
#' @seealso [tibble()] constructs a tibble from individual columnns. [enframe()]
#' @seealso [tibble()] constructs a tibble from individual columns. [enframe()]
#' which are no longer present in the result.
#' @param rownames How to treat existing row names of a data frame or matrix:
#' * `NULL`: remove row names.
#' * `NA`: keep row names. This the default, for compatibility, but a warning

This comment has been minimized.

@krlmlr

krlmlr Oct 22, 2018

Member

NULL is the new default, #114.

Show resolved Hide resolved R/as_tibble.R Outdated
Show resolved Hide resolved R/as_tibble.R Outdated
Show resolved Hide resolved R/tbl-df.r Outdated
Show resolved Hide resolved R/tbl-df.r Outdated
Show resolved Hide resolved R/tbl-df.r Outdated
Show resolved Hide resolved R/tbl-df.r
Show resolved Hide resolved R/tibble.R Outdated

jennybc and others added some commits Oct 22, 2018

Add backticks
Co-Authored-By: jennybc <jenny.f.bryan@gmail.com>
Add more backticks
Co-Authored-By: jennybc <jenny.f.bryan@gmail.com>
Auto-link to ?NROW
Co-Authored-By: jennybc <jenny.f.bryan@gmail.com>
Highlight the actual `[` usage that differs
Co-Authored-By: jennybc <jenny.f.bryan@gmail.com>
Whitespace
Co-Authored-By: jennybc <jenny.f.bryan@gmail.com>
Qualify with namespace
Co-Authored-By: jennybc <jenny.f.bryan@gmail.com>
Add a bullet re: syntactic names
Co-Authored-By: jennybc <jenny.f.bryan@gmail.com>
Auto-link to ?reserved
Co-Authored-By: jennybc <jenny.f.bryan@gmail.com>
@jennybc

This comment has been minimized.

Copy link
Member

jennybc commented Oct 22, 2018

OK @krlmlr I incorporated your feedback.

@jennybc

This comment has been minimized.

Copy link
Member

jennybc commented Oct 23, 2018

I am hunting down the documentation problem (seen on travis; appveyor is failing for some other reason unrelated to this PR), which leads to roxygen2, which potentially leads to commonmark. I think it is not a reason to delay merging.

@jennybc

This comment has been minimized.

Copy link
Member

jennybc commented Oct 23, 2018

I have resolved the documentation problem, so I think this is good to go.

@krlmlr

krlmlr approved these changes Oct 24, 2018

Copy link
Member

krlmlr left a comment

Thanks for working on this!

Show resolved Hide resolved R/tbl-df.r
Show resolved Hide resolved R/utils-format.r

@krlmlr krlmlr merged commit f951b40 into master Oct 24, 2018

2 of 4 checks passed

continuous-integration/appveyor/branch AppVeyor build failed
Details
continuous-integration/appveyor/pr AppVeyor build failed
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
continuous-integration/travis-ci/push The Travis CI build passed
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment