-
Notifications
You must be signed in to change notification settings - Fork 974
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
Consistent Error Handling for setnames()
with NA_character_
#6240
Conversation
Generated via commit 1968855 Download link for the artifact containing the test results: ↓ atime-results.zip Time taken to finish the standard R installation steps: 12 minutes and 13 seconds Time taken to run |
I am not sure about adding another error here, since actually We should also add documentation for how missing values are treated in setattr.Rd
my reading of the above is that Current doc for new arg is
I would keep the current behavior for back-compatibility and change the doc for new arg to below |
@@ -2619,6 +2619,7 @@ setnames = function(x,old,new,skip_absent=FALSE) { | |||
} else { | |||
w = which(names(x) != old | (Encoding(names(x)) != Encoding(old))) | |||
} | |||
if (anyNA(old)) stopf("NA in 'new' at positions %s", brackify(which(is.na(old)))) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
this code has variabled named old but the error says 'new' which is confusing, please clarify
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
does this code also add a new error when both old and new are specified?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
does this code also add a new error when both old and new are specified?
The thing is that this condition can only be reached when either old
or new
is missing, so it doesn't make changes to the condition when both old
and new
are specified.
This code has a variable named
old
, but the error message refers tonew
, which is confusing. Please clarify
When we do something like this:
setnames(DT, c('c', NA_character_))
we are inputting the new names in the old
argument hence old
variable carries the new names .
And when we do:
setnames(DT, new = c('c', NA_character_))
there is a condition that checks if old
is missing. If old
is missing, that means we are inputting the new names through the new
argument. In this condition, the value of new
is assigned to old
, and new
is set to NULL
, so new names end up in the old
variable.
hence, to check for NA
s in new names, we have to check the old
variable. This causes the error and variable mismatching, but since we are actually finding NA's in new names i think the error is correct but we can do something about the variable though...
We can fix this by modifying the code a bit, but that might cause some complications in some other places.
based on our discussion in #6236 (comment) I think this Pr should be closed and we should instead just add that sentence to the docs, is that OK? |
sure |
closes #6236
Summary:
This pull request addresses the inconsistency in the
setnames()
function when handlingNA_character_
in column renaming. Previously,setnames()
would error on partial assignments withNA_character_
but silently ignore the same in full assignments. This update ensures consistent error handling for both cases, providing a clear error message whenNA_character_
is used as a new column name.Changes Made:
setnames()
to error ifNA_character_
is provided as a name in both partial and full assignments.