-
Notifications
You must be signed in to change notification settings - Fork 991
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
Consistency in 'i' for integer and logical types #697
Comments
FR #633 title says "and maybe . and .. prefixes to symbols". There I was thinking . and .. on the symbols themselves. We do that anyway ourselves (manually) right? So it's really just building it in. ids = c("id1","id2")
DT[ids] # uses ids in calling scope because i is a single variable name
# on its own (rule in ?data.table). Common usage now.
DT[ logicalCol == TRUE ] # pretty clear, but possible mistakes if logicalCol
# isn't actually a column but is in calling scope
DT[ logicalCol ] # 'not found' currently but better error could suggest :
DT[ (logicalCol) ] # currently what I do to make it not a single name
DT[ .logicalCol ] # same, but error if no column is called "logicalCol"
# even if it does exist in calling scope (more robust)
DT[ colA > ..value ] # use 'value' in calling scope even if there's a column
# called "value". We do this manually currently by
# defining ..value in calling scope.
DT[ .colA > .colB ] # be explicit that colA and colB must be in DT
# otherwise error
DT[ colA > colB, strict.scope=TRUE] # same, without needing . prefixes
. and .. are really just an extension of the |
Okay, sounds good. |
On current master, this is the error:
For
So, it seems we could at least make the error message a bit more consistent:
|
Currently, there is one small inconsistency in row-subset using integer and logical types:
i
as integer:The same happens for
logical
type as well.The question is, can we get this indexing to work without the need for
(
for cases wherei
is a column inDT
instead of throwing the error?Edit: Cleaned up noise and retained just the resolution.
The text was updated successfully, but these errors were encountered: