Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

.SDcols peeling outer ( and edge case of :, and a logical of diff length #4470

Merged
merged 8 commits into from
Jun 22, 2021

Conversation

jangorecki
Copy link
Member

@jangorecki jangorecki commented May 21, 2020

closes #4231 and #4115

@codecov
Copy link

codecov bot commented May 21, 2020

Codecov Report

❗ No coverage uploaded for pull request base (master@7ac2120). Click here to learn what that means.
The diff coverage is 100.00%.

❗ Current head 8f4efe1 differs from pull request most recent head bac9868. Consider uploading reports for the commit bac9868 to get more accurate results
Impacted file tree graph

@@            Coverage Diff            @@
##             master    #4470   +/-   ##
=========================================
  Coverage          ?   99.60%           
=========================================
  Files             ?       72           
  Lines             ?    13920           
  Branches          ?        0           
=========================================
  Hits              ?    13865           
  Misses            ?       55           
  Partials          ?        0           
Impacted Files Coverage Δ
R/data.table.R 100.00% <ø> (ø)
src/assign.c 99.84% <ø> (ø)
src/between.c 100.00% <ø> (ø)
src/bmerge.c 100.00% <ø> (ø)
src/chmatch.c 100.00% <ø> (ø)
src/cj.c 100.00% <ø> (ø)
src/coalesce.c 100.00% <ø> (ø)
src/dogroups.c 100.00% <ø> (ø)
src/fastmean.c 100.00% <ø> (ø)
src/fcast.c 100.00% <ø> (ø)
... and 61 more

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 7ac2120...bac9868. Read the comment docs.

@mattdowle
Copy link
Member

mattdowle commented May 21, 2020

Great. The news item could be better. I try and include a code example (it doesn't have to be a reproducible example for a news item; just show the syntax), and where an error message is referred to, include the text of the error message. That way when a user searches for the error message, they find the news item.
I can make these changes to the news item.

@ColeMiller1
Copy link
Contributor

If we bring back #4248 or adjust your #4174 , we would address the issue while having a nice refactor of some [.data.table code.

@jangorecki jangorecki linked an issue May 25, 2020 that may be closed by this pull request
@jangorecki jangorecki changed the title .SDcols peeling outer ( and edge case of : .SDcols peeling outer ( and edge case of :, and a logical of diff length May 25, 2020
@mattdowle mattdowle added this to the 1.14.1 milestone Jun 22, 2021
R/data.table.R Outdated
@@ -1016,6 +1018,7 @@ replace_dot_alias = function(e) {
if (anyNA(.SDcols))
stop(".SDcols missing at the following indices: ", brackify(which(is.na(.SDcols))))
if (is.logical(.SDcols)) {
if (length(.SDcols)!=length(x)) warning("logical vector in .SDcols should have equal length to number of columns in input table")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We usually try and be even more helpful in error messages by including the values of length(x) and length(.SDcols) in the message.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

internal error when specifying .SDcols .SDcols with logical vector
3 participants