gt icon indicating copy to clipboard operation
gt copied to clipboard

Include the `accounting` option in `fmt_percent()` and `fmt_number()`

Open rich-iannone opened this issue 3 years ago • 8 comments

The accounting arg/option is only currently available in fmt_currency() but it's useful to have in fmt_percent() and even fmt_number(). Thanks @tareefk for the suggestion on this (for its inclusion in fmt_percent()).

rich-iannone avatar Nov 03 '20 16:11 rich-iannone

Hey, @rich-iannone -- when we were dealing with this some time ago, I did submit a PR for this (#306). It's a little out of date since the addition of sigfigs, but is pretty close. I'm happy to try to get the PR passing to see if that's helpful for this, since it does address fmt_percent and fmt_number.

steveputman avatar Nov 03 '20 16:11 steveputman

Thank you @steveputman , I think this time we will be more ready for this contribution!

rich-iannone avatar Nov 03 '20 16:11 rich-iannone

Regarding this, can we split "accounting" argument back into the way it used to be with one argument for negative value handling and one for currency symbol (or something similar that gives you the flexibility to do one BUT NOT the other)?. There is no way currently to say "wrap negative values in parenthesis ONLY, WITHOUT adding currency symbol as prefix" and vice versa.

Happy to open separate issue, let me know.

koppsp avatar Jan 12 '21 00:01 koppsp

@koppsp I know the formatters have changed over time (and I'm unsure yet whether I've made any breaking changes with respect to all the other functionality @rich-iannone and team have added on the formatters), but--assuming I'm not missing what you're after here--I deal with the issue you face with the choice of formatter call. To use your examples, if you want parentheses but no currency symbol, you would use fmt_number(. . ., accounting = TRUE), whereas if you want the currency symbol but not the parentheses for negative values, you'd use fmt_currency(. . ., accounting = FALSE). Maybe the option should be renamed, because most people (and Excel) associate currency symbols with accounting.

steveputman avatar Jan 12 '21 21:01 steveputman

@steveputman Thanks for the response!

If you are saying that right now I can use fmt_number(. . ., accounting = TRUE), 0.2.2 function does not have this argument.

fmt_number(
  data,
  columns,
  rows = NULL,
  decimals = 2,
  n_sigfig = NULL,
  drop_trailing_zeros = FALSE,
  drop_trailing_dec_mark = TRUE,
  use_seps = TRUE,
  scale_by = 1,
  suffixing = FALSE,
  pattern = "{x}",
  sep_mark = ",",
  dec_mark = ".",
  locale = NULL
)

If you are saying that it could potentially be designed that way, then yeah that would/could/should work. I personally don't see a big difference between a number and a currency besides the currency symbol prefix for currency. I'd recommend making one function and leave it up to the user to "turn the numbers into currency type values" via the function's provided arguments.

koppsp avatar Jan 13 '21 16:01 koppsp

Right: this is still an open issue, and it doesn't work that way now, but that's how it works in the PR referenced higher in the thread (#306).

steveputman avatar Jan 13 '21 16:01 steveputman

@steveputman Thanks for being patient and continuing to contribute to your PR for this issue. I promise that the stars will align on this one and your contribution will be reviewed and merged.

We took a half measure to include the accounting option in fmt_number() and fmt_percent() because there was some urgency to at least minimally support it in v0.3.0. For the next release, the focus will be on the alignment of parens problem that you took on.

rich-iannone avatar May 12 '21 16:05 rich-iannone

@rich-iannone Ha no problem (and not 100% altruistic since it keeps me enjoying the benefits of your new features while still being able to deal with my edge case!)

steveputman avatar May 12 '21 17:05 steveputman