Add a by attribute to data_tabulate() objects when by is present - #690
Conversation
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.
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.
There was a problem hiding this comment.
Ok I'm convinced.
(Re your comment about "Requested changes" below, this is because Github keeps this label until the same person approves the PR.)
This comment was marked as off-topic.
This comment was marked as off-topic.
|
wrong branch |
|
I'm not sure if there is a change I missed but it is still showing one requested change . I can't tell what it is or if it was actually resolved. |
|
Please remember to increase the version number in the fourth place by one for "user visible" changes (e.g. to 1.3.1.x) in the description file. This ensures that |
3 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