Fix #7354, Implementing NSE in cube#7603
Fix #7354, Implementing NSE in cube#7603sisyphuswastaken wants to merge 5 commits intoRdatatable:masterfrom
Conversation
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## master #7603 +/- ##
=======================================
Coverage 99.02% 99.03%
=======================================
Files 87 87
Lines 16897 16939 +42
=======================================
+ Hits 16733 16775 +42
Misses 164 164 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
This comment was marked as resolved.
|
I've added new tests for code coverage, and tried to undo the unnecessary changes that occurred when I had replaced the full function while integrating |
|
Hello! Bumping on this, let me know if any changes are to be made to the PR, I'd be happy to adjust :) @jangorecki |
| error = "\\.SDcols is a logical vector of length" | ||
| ) | ||
|
|
||
| test(1750.38, |
There was a problem hiding this comment.
Tests should be made against pre-computed data. Otherwise we can mis a bug if it will be impacting both LHS and RHS of a tested code
| } | ||
|
|
||
| # Helper function to process SDcols | ||
| .processSDcols = function(SDcols_sub, SDcols_missing, x, jsub, by, enclos = parent.frame()) { |
There was a problem hiding this comment.
I think the point to have helper function was to use it in [.data.table() too, or am I missing something?
And then I don't see it being used there.
The comment was in previous, now abandoned, PR: #7543 (comment)
Pertains to issue #7354.
Changes implemented:
This PR continues work from Implementing NSE in cube #7540 and and Implementing NSE in cube #7543.