names<- retains sorting attributes #5849
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #5849 +/- ##
=======================================
Coverage 97.46% 97.47%
=======================================
Files 80 80
Lines 14822 14823 +1
=======================================
+ Hits 14447 14448 +1
Misses 375 375 ☔ View full report in Codecov by Sentry. |
This comment was marked as resolved.
This comment was marked as resolved.
| @@ -18111,3 +18111,11 @@ test(2238.9, NA %notin% c(1:5, NA), FALSE) | |||
|
|
|||
| # shift actionable error on matrix input #5287 | |||
| test(2239.1, shift(matrix(1:10, ncol = 1)), error="consider wrapping") | |||
|
|
|||
| # names<- retains index and key, #5125, #5126, #5126, #5128 | |||
There was a problem hiding this comment.
good idea. I skipped originally bc I wasn't sure whether output change tests are worth copying.
I'll read more carefully and copy over the worthwhile tests.
There was a problem hiding this comment.
OK, copied over the tests. They caught two things:
colnames(x) <-is now treated the same anames(x) <-. Requires a new S3 method registration.- Doing
names(x) <- "..."; set(x, j=j, val=val)is fixed with a simplesetalloccol(). Maybe not ideal, but a good expedient.
There was a problem hiding this comment.
I don't see any tests being commented out. Does it mean we cover all tests from Matt's PR?
There was a problem hiding this comment.
I didn't copy over anything where the only diff is in error/message output, i.e.
The new tests of actual code are copied over and pass:
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as off-topic.
|
Will go ahead and merge, but it would be helpful if some of the commenters on the original PR #5133 (@ben-schwen @ColeMiller1) could chime in on whether we're leaving something on the table vs. the original intent of that PR, in other words, what should we target as follow-up to achieve the original goals of #5133? Or should we consider the matter resolved & close that PR too? |
Follow up to #5084
Related to #5085
Closes #5125
Closes #5126
Closes #5127
Closes #5128
Minimized successor to #5133. That PR has a lot going on that I don't follow -- for now I'm focused on fixing the broken revdeps so we can release to CRAN.
I've confirmed {CornerstoneR}, {maditr}, {getDTeval}, and {chicane} have no test failures after this fix.
Revdep {simDAG} (as noted here: #5128 (comment)) had 5 errors, this PR removes the 2 related to attributes. The remaining three are covered by #5680.