Closes #2887 -- adds a typo checker to i for convenience

[MichaelChirico]

Pretty rudimentary, just to get things started.

A few points for discussion/improvement:

• I eschewed lassoing in objects from .GlobalEnv for now, but I left .checkTypos flexible enough that it should be easy to expand this (by either sending c(names(x), ls(envir = .GlobalEnv)) as the ref argument, or by splitting into ref_x and ref_global_env to allow the error to distinguish between objects found in the scope of x vs. parent scope)
• This is only one place in i. If we think it's worth it, could add similar code in other places like here or even here
• Nothing done in j or by for now.
• Anyone who's more familiar/accustomed to using agrep might be able to suggest different defaults for the cost value(s).

[MichaelChirico]

this is irrelevant to this PR, but I was getting a NOTE from R CMD check about an overfull line

[jangorecki]

LGTM, but better wait with it till 1.12.0
@MichaelChirico maybe some more test for column names including spaces and special characters?

[MichaelChirico]

agree, we should keep it as experimental in dev for a while.

…

On Fri, Jun 15, 2018, 12:37 AM Jan Gorecki ***@***.***> wrote: ***@***.**** approved this pull request. LGTM, but better wait with it till 1.12.0 — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#2891 (review)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AHQQdYaw3oDaf9YFW6v1Gj7cO7fYOS3rks5t8pFEgaJpZM4UFjNr> .

[st-pasha]

2 suggestions:

1. Add a special case for length(found) == 1 ("one of ..." seems awkward for 1 item).
2. Limit the number of columns suggested to 3 or 4 (in theory your DT can have 1000s of columns, similarly named, and all of them might be found by agrep).

[st-pasha]

Similarly here ref can have an enormous size.

[MichaelChirico]

a very reasonable case for this is columns like V1,...,Vn or in a wide table like income00, income01, ..., income18. I thought of putting in a limit to prevent error explosion, but R does that itself https://stackoverflow.com/a/50387968/3576984 OTOH I guess the user should get the point... if they put in Vx and are matched to V1,...,V99, seeing "V1,V2,V3,..." should be sufficient.

…

On Fri, Jun 15, 2018, 7:54 AM Pasha Stetsenko ***@***.***> wrote: ***@***.**** commented on this pull request. ------------------------------ In R/data.table.R <#2891 (comment)> : > @@ -200,6 +200,21 @@ replace_dot_alias <- function(e) { x } +.checkTypos = function(err, ref) { + if (grepl('object.*not found', err$message)) { + used = gsub(".*object '([^']+)'.*", "\\1", err$message) + found = agrep(used, ref, value = TRUE, ignore.case = TRUE, fixed = TRUE) + if (length(found)) { + stop("Object '", used, "' not found. Perhaps you ", + sprintf("intended one of: [%s]", + paste(found, collapse = ', '))) 2 suggestions: 1. Add a special case for length(found) == 1 ("one of ..." seems awkward for 1 item). 2. Limit the number of columns suggested to 3 or 4 (in theory your DT can have 1000s of columns, similarly named, and all of them might be found by agrep). ------------------------------ In R/data.table.R <#2891 (comment)> : > @@ -200,6 +200,21 @@ replace_dot_alias <- function(e) { x } +.checkTypos = function(err, ref) { + if (grepl('object.*not found', err$message)) { + used = gsub(".*object '([^']+)'.*", "\\1", err$message) + found = agrep(used, ref, value = TRUE, ignore.case = TRUE, fixed = TRUE) + if (length(found)) { + stop("Object '", used, "' not found. Perhaps you ", + sprintf("intended one of: [%s]", + paste(found, collapse = ', '))) + } else { + stop("Object '", used, "' not found among: ", + sprintf("[%s]", paste(ref, collapse = ', '))) Similarly here ref can have an enormous size. — You are receiving this because you authored the thread. Reply to this email directly, view it on GitHub <#2891 (review)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AHQQddH-PUHJDAskuxJU__V9wCARvmz6ks5t8ve4gaJpZM4UFjNr> .

[MichaelChirico]

awesome, thanks!

[mattdowle]

@MichaelChirico Did you benchmark to see if this slowed things down? It's an (expensive) tryCatch on most [.data.table calls that use i iiuc? Is there a way to avoid tryCatch?

We don't mind too much about call overhead normally, but tryCatch has the potential to add unreasonably to overhead, perhaps.

[MichaelChirico]

let me investigate...

…

On Fri, Sep 7, 2018, 7:43 AM Matt Dowle ***@***.***> wrote: @MichaelChirico <https://github.com/MichaelChirico> Did you benchmark to see if this slowed things down? It's an (expensive) tryCatch on most [.data.table calls that contains i iiuc? Is there a way to avoid tryCatch? — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub <#2891 (comment)>, or mute the thread <https://github.com/notifications/unsubscribe-auth/AHQQdd9FNqnmwfHkfOPospGIVeMeBzmnks5uYbM6gaJpZM4UFjNr> .

[st-pasha]

Possibly tryCatch can be moved inside .massagei -- in that case, the overhead can be avoided if i is ., which is probably the most common case.