Add a by attribute to data_tabulate() objects when by is present#690
Add a by attribute to data_tabulate() objects when by is present#690elinw wants to merge 11 commits into
by attribute to data_tabulate() objects when by is present#690Conversation
etiennebacher
left a comment
etiennebacher
left a comment
There was a problem hiding this comment.
Thanks! Yes please add at least one test for this.
| class(out) <- c("datawizard_tables", "list") | ||
| } else { | ||
| class(out) <- c("datawizard_crosstabs", "list") | ||
| attr(out, "by") <- gsub('\\"', "", by_name, fixed = TRUE) |
There was a problem hiding this comment.
Why is the gsub() necessary?
There was a problem hiding this comment.
Because in this case (but not the other) the name returns e.g. "/"Island"/". I'm not sure why or if there is a way to get the name without the escapes. I tried some ideas but there was nothing obvious to me.
There was a problem hiding this comment.
Meaning when a quoted variable name is used for by.
There was a problem hiding this comment.
Ok, should this also remove ' if one passes 'Island' in by?
There was a problem hiding this comment.
I also don't think \\ is necessary if fixed = TRUE is used
There was a problem hiding this comment.
And actually I stupidly followed the advice from the tester and added fixed = TRUE but that was wrong, this is not fixed it just looks like fixed. So the next push will put that back.
There was a problem hiding this comment.
Yeah it's not a bad instinct but it needs the escaping to work for whatever reason. But in the update I'm about to send you will see a change.
I just tried it again and it makes the tests fail to use fixed=TRUE
There was a problem hiding this comment.
Sorry but I don't see a case where using the backslash changes something:
x <- insight::safe_deparse("foo")
x
#> [1] "\"foo\""
gsub('"', "", x)
#> [1] "foo"
gsub('"', "", x, fixed = TRUE)
#> [1] "foo"
gsub('\\"', "", x)
#> [1] "foo"Do you have an example where this affects the output?
| class(out) <- c("datawizard_tables", "list") | ||
| } else { | ||
| class(out) <- c("datawizard_crosstabs", "list") | ||
| attr(out, "by") <- gsub('\\"', "", by_name, fixed = TRUE) |
etiennebacher
changed the title
by attribute to 1data_tabulate() objects when by` is present
etiennebacher
changed the title
by attribute to 1data_tabulate() objects when by` is presentby attribute to data_tabulate() objects when by is present
|
Also the linter wanted me to make some changes and then not make them (when I made them it told me to change them back) unless I misunderstood. |
| if (is.null(by)) { | ||
| class(out) <- c("datawizard_tables", "list") | ||
| } else { | ||
| out <- lapply(out, structure, by = by_name) |
There was a problem hiding this comment.
I don't understand why this attribute needs to be added to each element in out. Below we add the attribute collapse to out only, why can't we do that for by since it has the same value for all elements in out anyway?
There was a problem hiding this comment.
Yes this is what took me a long time to figure out. "by" is really a property of individual tables not of the list. This should be consistent whether you get the table through the data.frame method or through the default method or if select has one or multiple columns. Also it should be the same for grouped data frames.
It took me a while to see this, which was why I would get the tests to work on default and then they would fail or data.frame or with multiple columns.
As a practical matter, if you pull out one table from the list you should get the "by" along with it.
|
|
||
| test_that("data_tabulate data.frame by", { | ||
| data(efc, package = "datawizard") | ||
| x <- data_tabulate.data.frame(efc, "c172code", by = "e16sex") |
There was a problem hiding this comment.
| x <- data_tabulate.data.frame(efc, "c172code", by = "e16sex") | |
| x <- data_tabulate(efc, "c172code", by = "e16sex") |
data_tabulate() is an S3 generic so it should be dispatched automatically to data_tabulate.data.frame() when the input is a data.frame.
2 participants
I wasn't sure if you would want an additional test; all the current tests are still passing.
The attributes aren't listing in the documentation so I didn't change anything there.
Closes #688