Skip to content

Conversation

@krlmlr
Copy link
Collaborator

@krlmlr krlmlr commented Mar 17, 2021

We want to know that a formattable is actually "accounting" or "currency" after we have constructed it. This helps implementing pillar_shaft() methods for formattable.

Are you on board with this? Are there reasons against? I ran a revdepcheck: https://github.com/renkun-ken/formattable/actions/runs/660611546. Two packages are now failing but it's unrelated to this PR because we no longer import htmltools and rmarkdown.

This is a big change I couldn't break into smaller pieces.

@krlmlr krlmlr requested a review from renkun-ken March 17, 2021 09:37
@renkun-ken
Copy link
Owner

renkun-ken commented Mar 17, 2021

I'll take a closer look at this soon.

R/val_prefix.R Outdated
prefix <- function(x, prefix = "", sep = "", ..., na.text = NULL) {
formattable(x, ...,
formattable(x,
class = "formattable_prefix",
Copy link
Owner

Choose a reason for hiding this comment

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

Is there a reason prefix and suffix uses a class? These two functions are intended to tweak existing formats.

I inspect the class attribute like the following:

> str(suffix(prefix(currency(acct, symbol = "HK$"), "*"), "#"))                                                                                                                                                                                                                                                                          
 'formattable_accounting' num [1:3] *HK$10,000.00# *HK$500,000.00# *HK$-20,000.00#
 - attr(*, "formattable")=List of 5
  ..$ formatter: chr "formatC"
  ..$ format   :List of 3
  .. ..$ format  : chr "f"
  .. ..$ big.mark: chr ","
  .. ..$ digits  : int 2
  ..$ preproc  : NULL
  ..$ postproc :List of 3
  .. ..$ :function (str, x)  
  .. ..$ :function (str, x)  
  .. ..$ :function (str, x)  
  ..$ class    : chr [1:4] "formattable_suffix" "formattable_accounting" "formattable" "numeric"

However, the prefix class seems missing somehow?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Good point, this doesn't look right. Maybe prefix and suffix don't need their own class.

@renkun-ken
Copy link
Owner

A bit off the topic. @krlmlr Please feel free to add yourself as a contributor to DESCRIPTION to reflect your active participation of the package. Thanks!

@krlmlr krlmlr requested a review from renkun-ken April 15, 2021 02:43
@krlmlr
Copy link
Collaborator Author

krlmlr commented Apr 15, 2021

pillar 1.6.0 is on CRAN now.

@renkun-ken
Copy link
Owner

One thing to notice is that the class name is bit too long for printing:

> library(formattable)
> library(data.table)
> library(tibble)

> p <- data.frame( 
    id = c(1, 2, 3, 4, 5), 
    name = c("A1", "A2", "B1", "B2", "C1"), 
    balance = accounting(c(52500, 36150, 25000, 18300, 7600), format = "d"), 
    growth = percent(c(0.3, 0.3, 0.1, 0.15, 0.15), format = "d"), 
    ready = formattable(c(TRUE, TRUE, FALSE, FALSE, TRUE), "yes", "no"))                                                                                                                                                                                                                              

> p                                                                                                                                                                                                                                                                                                   
  id name balance growth ready
1  1   A1  52,500    30%   yes
2  2   A2  36,150    30%   yes
3  3   B1  25,000    10%    no
4  4   B2  18,300    15%    no
5  5   C1   7,600    15%   yes

> as_tibble(p)                                                                                                                                                                                                                                                                                        
# A tibble: 5 x 5
     id name  balance    growth     ready     
  <dbl> <chr> <frmttbl_> <frmttbl_> <frmttbl_>
1     1 A1    52,500      30%       TRUE      
2     2 A2    36,150      30%       TRUE      
3     3 B1    25,000      10%       FALSE     
4     4 B2    18,300      15%       FALSE     
5     5 C1    7,600       15%       TRUE      

> as.data.table(p)                                                                                                                                                                                                                                                                                    
      id   name                  balance                growth                 ready
   <num> <char> <formattable_accounting> <formattable_percent> <formattable_logical>
1:     1     A1                   52,500                   30%                   yes
2:     2     A2                   36,150                   30%                   yes
3:     3     B1                   25,000                   10%                    no
4:     4     B2                   18,300                   15%                    no
5:     5     C1                    7,600                   15%                   yes

Do you think we should simplify or shorten the class name?

@krlmlr
Copy link
Collaborator Author

krlmlr commented Apr 15, 2021

We could also add pillar_shaft() and type_sum() methods, perhaps as a separate PR?

@renkun-ken
Copy link
Owner

We could also add pillar_shaft() and type_sum() methods, perhaps as a separate PR?

A separate PR makes good sense.

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

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants