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

support vctrs abbreviations #1927

Merged
merged 2 commits into from
Oct 18, 2020
Merged

support vctrs abbreviations #1927

merged 2 commits into from
Oct 18, 2020

Conversation

sebkopf
Copy link
Contributor

@sebkopf sebkopf commented Oct 16, 2020

Suggested fix for #1487 , this follows the equivalent use of vctrs::vec_ptype_abbr in the pillar package (see code).

Note that it would be possible to replace the paged_table_type_sum() function almost entirely with what is implemented broadly by the vctrs::vec_ptype_abbr generic (see e.g. implementation for atomic data types, date time, and factors) but it would introduce small differences (e.g. the factor abbreviation would become fct instead of fctr) and I wasn't sure if this is something you'd be interested in.

ps: since this is my first pull request to rstudio, I have sent the signed contributor agreement to jj@rstudio.com

suggested fix for rstudio#1487 , this follows the implementation of vctrs support in the pillar package: https://github.com/r-lib/pillar/blob/master/R/type-sum.R#L38
@CLAassistant
Copy link

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you sign our Contributor License Agreement before we can accept your contribution.


Sebastian Kopf seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.
You have signed the CLA already but the status is still pending? Let us recheck it.

Copy link
Member

@yihui yihui left a comment

Choose a reason for hiding this comment

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

Thanks!

DESCRIPTION Outdated
@@ -91,7 +91,8 @@ Imports:
tinytex (>= 0.11),
xfun (>= 0.15),
methods,
stringr (>= 1.2.0)
stringr (>= 1.2.0),
vctrs (>= 0.3.2)
Copy link
Member

Choose a reason for hiding this comment

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

I'm hesitant on introducing a new dependency only for the purpose of supporting a specific feature html_document(df_print = "paged"). I'm okay if it is in Suggests, and we call it conditionally, e.g., if (xfun::loadable('vctrs')) return(vctrs:: vec_ptype_abbr(x)).

If the difference between the output of rmarkdown:::paged_table_type_sum() and vctrs::vec_ptype_abbr() is minor, I tend to fix the difference to make them consistent.

Copy link
Contributor Author

@sebkopf sebkopf Oct 17, 2020

Choose a reason for hiding this comment

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

Thank you, I moved vctrs to suggests and included the check whether the package is installed. This should work well since anyone who wants to make use of vctrs in a paged table will have it installed anyways. Could also skip the suggests altogether since tibble is already on the suggests list for rmarkdown and tibble depends on vctrs.

With this, these kind of vctrs-type vector implementations now print similarly in standard output and paged tables:

library(vctrs)
percent <- function(x = double()) {
  vec_assert(x, double())
  new_vctr(x, class = "percent")
}
format.percent <- function(x, ...) {
  paste0(formatC(signif(vec_data(x) * 100, 3)), "%")
}
vec_ptype_abbr.percent <- function(x, ...) "prcnt"

df <- tibble::tibble(x = percent(runif(4)))

in a knitted R markdown file, df prints as:

## # A tibble: 4 x 1
##         x
##   <prcnt>
## 1   14.1%
## 2   51.5%
## 3   65.5%
## 4   95.5%

and rmarkdown::paged_table(df) now also as

screenshot

Copy link
Member

@yihui yihui left a comment

Choose a reason for hiding this comment

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

Great. Thanks!

@yihui yihui merged commit 7239cea into rstudio:master Oct 18, 2020
@cderv
Copy link
Collaborator

cderv commented Oct 18, 2020

Is this a confidential feature or should we add a bullet to NEWS ? 😄

@yihui
Copy link
Member

yihui commented Oct 18, 2020

Not confidential but I thought it was a rather minor change and probably not worth mentioning :) Please feel free to add a bullet.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Apr 20, 2021
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.

4 participants