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

updating comma_format, percent_format and unit_format #146

Merged
merged 1 commit into from Jul 8, 2018

Conversation

Projects
None yet
2 participants
@larmarange
Contributor

larmarange commented Jul 6, 2018

now based on number_format()

cf. discussion in #142

@larmarange

This comment has been minimized.

Contributor

larmarange commented Jul 6, 2018

Solve:

@larmarange

This comment has been minimized.

Contributor

larmarange commented Jul 6, 2018

Notes:

  • NEWS.md not yet updated
  • for unit_format and percent_format, I kept the default value of big.mark used in number_format (i.e. a space) for consistency
  • for unit_format, I kept unit and sep arguments for backward compatibility. Maybe it could be relevant to add in documentation that unit_format is deprecated and that number_format could be used instead.
@hadley

This comment has been minimized.

Member

hadley commented Jul 6, 2018

Can you please use Fixes #140? That specific formulation will cause the related issue to be closed when this PR is merged.

@hadley

Looking good.

I think it's worth considering if we do want to add ... to each of the existing formatters. Would it be simpler (and easier to document) to simply tell people to use number_format() if they need more control?

#'
#' @return `number_format` returns a function with single parameter
#' `number_format` and `number` are generic formatters for numbers.

This comment has been minimized.

@hadley

hadley Jul 6, 2018

Member

Can you please use () for functions?

This comment has been minimized.

@larmarange

larmarange Jul 6, 2018

Contributor

done

#' @param x a numeric vector to format
#' @return a function with single parameter x, a numeric vector, that
#' returns a character vector
#' `comma_format` and `comma` format numbers with commas separating thousands.

This comment has been minimized.

@hadley

hadley Jul 6, 2018

Member

I think it's easier to understand if you put all the description in one place.

(But the other tags should stay with individual functions)

This comment has been minimized.

@larmarange

larmarange Jul 6, 2018

Contributor

ok. I have also reordered the code (i.e. each function being declared in the R file in the same order as the doc).

#' point(c(1, 1e3, 2000, 1e6))
#' point(c(1, 1.021, 1000.01))
comma_format <- function(...) {
comma_format <- function(big.mark = ",", ...) {
force_all(...)

This comment has been minimized.

@hadley

hadley Jul 6, 2018

Member

I think this force all can be removed.

#' french_percent(runif(10))
#'
percent_format <- function(accuracy = NULL, scale = 100, suffix = "%", ...) {
force_all(...)

This comment has been minimized.

@hadley

hadley Jul 6, 2018

Member

force_all() is only needed in functions that return functions.

This comment has been minimized.

@larmarange

larmarange Jul 6, 2018

Contributor

ok it has been removed.

by security, I added it in number_format()

#' )
#' french_percent(runif(10))
#'
percent_format <- function(accuracy = NULL, scale = 100, suffix = "%", ...) {

This comment has been minimized.

@hadley

hadley Jul 6, 2018

Member

Is there a reason to make scale or suffix parameters?

This comment has been minimized.

@hadley

hadley Jul 6, 2018

Member

Oh, I see for per_mille. I think it would be simpler to continue to use number_format() for that example.

#' comma_format()(c(1, 1e3, 2000, 1e6))
#' comma_format(digits = 9)(c(1, 1e3, 2000, 1e6))
#' comma_format(accuracy = .01)(c(1, 1e3, 2000, 1e6))

This comment has been minimized.

@hadley

hadley Jul 6, 2018

Member

We probably do need to preserve the digits argument for backward compatibility.

paste(comma(x * scale, ...), unit, sep = sep)
}
#'
unit_format <- function(unit = "m", sep = " ", suffix = paste0(sep, unit), ...) {

This comment has been minimized.

@hadley

hadley Jul 6, 2018

Member

I think it's still worth including scale here, just to be explicit.

This comment has been minimized.

@larmarange

larmarange Jul 6, 2018

Contributor

done

@larmarange

This comment has been minimized.

Contributor

larmarange commented Jul 6, 2018

Regarding allowing ... in percent format()

If I have to use number_format for displaying percentage with a comma as a decimal separator, I need to re-declare all options, i.e. number_format(decimal.mark = ",", scale = 100, accuracy = NULL, suffix = "%") while using percent(big.mark = ",") is much easier and intuitive.

Regarding digits argument in comma_format()

In the current implementation of comma_format(), digits should be understood as in format() (i.e. total significant numbers to display, before or after the decimal point) and not the number of figures after the decimal point as in round().

> comma(123.45678)
[1] "123.4568"
> comma(123.45678, digits = 0)
[1] "123"
> comma(123.45678, digits = 2)
[1] "123"
> comma(123.45678, digits = 5)
[1] "123.46"

That behaviour is not very intuitive.

In addition, it will require some code to determine the corresponding value of accuracy to be passed to number_format or it will require some changes in number_format() by adding a digits argument and to not take into account the accuracy argument if digits is supplied. Not sure that there is a lot of situations where digits is useful in that sense.

I could understand a digits argument corresponding to the number of figures to display after the decimal point (like in round()) and then to fix accuracy accordingly, but such behaviour will not maintain backward compatibility for comma.

Regarding the ... argument for other formatters than number_format

If we keep in mind that ordinal_format or dollar_format could also rely on number_format with some extra functionalities, then it will be relevant to keep ... for those functions. Wouldn't it be more consistent to keep ... in all formatters then?

@larmarange larmarange force-pushed the larmarange:comma_percent_unit branch from 6c8a920 to b55b5ee Jul 6, 2018

@hadley

This comment has been minimized.

Member

hadley commented Jul 6, 2018

  • Ok.

  • Ok. In that case, we should formally deprecate the argument, warning() if it is !missing(), and suggesting to use accuracy instead

  • The alternative is to explicitly pass along every argument. This is more work for the developer, but it makes life for easier for the user (because they get autocomplete on function arguments, and the documentation doesn't need to be read as closely to figure out what happens with ...)

@larmarange larmarange force-pushed the larmarange:comma_percent_unit branch from b55b5ee to 7bca5fc Jul 7, 2018

@larmarange

This comment has been minimized.

Contributor

larmarange commented Jul 7, 2018

I have updated all functions to pass all arguments.

A warning has been added to comma() and comma_format.

@hadley

Looks good. Just two small style fixes, and a news bullet, and it'll be good to merge.

suffix = "", big.mark = ",", decimal.mark = ".",
trim = TRUE, digits, ...) {
if (!missing(digits)) {
warning("'digits' argument is deprecated, use 'accuracy' instead.")

This comment has been minimized.

@hadley

hadley Jul 7, 2018

Member

Needs call. = FALSE, and instead of 'digits' can you please do `digits`? (And similarly for accuracy)

This comment has been minimized.

@larmarange

larmarange Jul 7, 2018

Contributor

done

#'
#' @return `number_format` returns a function with single parameter
#' `number_format()` and `number()` are generic formatters for numbers.

This comment has been minimized.

@hadley

hadley Jul 7, 2018

Member

Maybe add something like: "All formatters allow you to re-`scale` (multiplicatively) and round to specified `accuracy`"

This comment has been minimized.

@larmarange

larmarange Jul 7, 2018

Contributor

done

@larmarange larmarange force-pushed the larmarange:comma_percent_unit branch from 7bca5fc to acd202c Jul 7, 2018

@larmarange

This comment has been minimized.

Contributor

larmarange commented Jul 7, 2018

All fixed and news bullet added

@larmarange larmarange force-pushed the larmarange:comma_percent_unit branch from acd202c to b32306a Jul 7, 2018

@hadley

A couple more tiny suggstions. Also need to fix the merge conflict, then I can merge.

#'
#' @return `number_format` returns a function with single parameter
#' `number_format()` and `number()` are generic formatters for numbers.\cr

This comment has been minimized.

@hadley

hadley Jul 7, 2018

Member

Please eliminate the \cr - these should all run into together in one paragraph. (Or if you don't like that, use a bulleted list instead)

This comment has been minimized.

@hadley

hadley Jul 7, 2018

Member

Or maybe you want to use @description so you can use regular new lines?

This comment has been minimized.

@larmarange

larmarange Jul 8, 2018

Contributor

done

trim = TRUE, ...) {
force_all(...)

This comment has been minimized.

@hadley

hadley Jul 7, 2018

Member

Needs to include all args

This comment has been minimized.

@larmarange

larmarange Jul 8, 2018

Contributor

done

@larmarange larmarange force-pushed the larmarange:comma_percent_unit branch from b32306a to 756d372 Jul 8, 2018

@larmarange

This comment has been minimized.

Contributor

larmarange commented Jul 8, 2018

merge conflict solve and corrections made

@hadley hadley merged commit 4122d63 into r-lib:master Jul 8, 2018

3 checks passed

codecov/patch 89.87% of diff hit (target 67.11%)
Details
codecov/project 68.37% (+1.26%) compared to 32e6d55
Details
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@hadley

This comment has been minimized.

Member

hadley commented Jul 8, 2018

Thanks!

@larmarange larmarange deleted the larmarange:comma_percent_unit branch Jul 9, 2018

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment