bring add_totals functions up to v1.0 as add_totals #99
Conversation
still need to deprecate the old ones
deprecated functions cause warnings in tests
Failing with R-devel because some packages aren't available
Codecov Report
@@ Coverage Diff @@
## master #99 +/- ##
=====================================
Coverage 100% 100%
=====================================
Files 14 14
Lines 310 325 +15
=====================================
+ Hits 310 325 +15
Continue to review full report at Codecov.
|
|
|
also redirect add_totals_row and _col calls through add_totals()
There was a problem hiding this comment.
Looks good! Great stuff. Some of the code feels heavy but we are not aiming for lightweight here so I think that's fine. Overall I think I prefer an adorn_ approach to things that modify a tabyl, but have shared that before and support whichever approach you decide to take.
R/add_totals.R
Outdated
|
|
||
| if("col" %in% which){ | ||
| # Add totals col | ||
| clean_dat <- clean_names(dat) # bad names will make select_if choke |
There was a problem hiding this comment.
Reference the relevant dplyr issue here; this should be temporary
| dat | ||
| } | ||
|
|
||
| ### Deprecated functions ----------------------------- | ||
| #' @title Append a totals row to a data.frame. | ||
| #' | ||
| #' @description |
There was a problem hiding this comment.
Should the help reference that it is deprecated?
There was a problem hiding this comment.
Yep, good call. I'll add that.
- Deprecate the retired
add_totals_*functions in their documentation
R/ns_to_percents.R
Outdated
| @@ -18,32 +19,32 @@ | |||
| #' # when total_n is needed | |||
| #' mtcars %>% | |||
| #' crosstab(am, cyl) %>% | |||
| #' add_totals_row() %>% # add a totals row that should not be included in the denominator | |||
| #' add_totals("row") %>% # add a totals row that should not be included in the denominator | |||
| #' ns_to_percents(denom = "all", total_n = nrow(mtcars)) # specify correct denominator | |||
|
|
|||
| ns_to_percents <- function(dat, denom = "row", na.rm = TRUE, total_n = NULL){ | |||
There was a problem hiding this comment.
One option for a v1.0 approach:
This is a function primarily intended to modify a tabyl, so it would be an adorn function, probably adorn_percent(denom="row") for the percent of total over rows.
If you included both adorn_N and adorn_percent in the pipe after a tabyl, it would behave like adorn_crosstab does now, or maybe just add columns for both if that's easier.
It sounds hard to make tably create an environment that could allow for multiple adorn_ functions to be stacked. I think it's possible, but I have no idea how to do it.
R/add_totals.R
Outdated
| #' add_totals() | ||
|
|
||
|
|
||
| add_totals <- function(dat, which = c("row", "col"), fill = "-", na.rm = TRUE){ |
There was a problem hiding this comment.
I prefer adorn_totals as this is something that is primarily intended to modify a tabyl. I think that adorn_crosstab may eventually become the outlier in the adorn_ approach. I think I've given this feedback already, though, so I assume that you have heard it and decided on verb_object approach to things that modify a tabyl.
There was a problem hiding this comment.
I think there's something to this, and I agree that adorn_crosstab will become the outlier (a) because the nomenclature of "crosstab" may go away and (b) if we can fully replicate its functionality with modular pieces like this. In that case, I do like the common prefix of adorn_ and could see this as adorn_totals.
I want to merge this in now rather than wait for us to figure out #101, since this closes existing bugs. I'm not sure if it should be add_totals when we might later re-route people to adorn_totals... maybe adorn_totals now. I think this won't be set in stone until we resolve #101, the big question, and so no matter what we call it now, the name and behavior of the totals function could ultimately change. Say, if the totals function needs to add an attribute to a tabyl to convey info to ns_to_percents down the line.
There was a problem hiding this comment.
okay, I renamed it adorn_totals.
Closes #57, lots of discussion there.