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

Improved error messages #907

Merged
merged 18 commits into from
Jul 31, 2022
Merged

Improved error messages #907

merged 18 commits into from
Jul 31, 2022

Conversation

mgirlich
Copy link
Collaborator

@mgirlich mgirlich commented Jun 1, 2022

This is the first step to much more helpful error messages.

This fixes #803, fixes #905, and parts of #822.

  • Unknown variables now produce an error. I don't think there are many (or even any?) reasons that this should not produce an error.
  • fix_call() is currently needed because tidyselect/vctrs do not always handle the call correctly.

@mgirlich mgirlich added this to the 2.3.0 milestone Jun 1, 2022
@@ -20,7 +20,7 @@
#' library(dplyr, warn.conflicts = FALSE)
#'
#' lf <- lazy_frame(a = TRUE, b = 1, c = 2, d = "z", con = simulate_hana())
#' lf %>% transmute(x = paste0(z, " times"))
#' lf %>% transmute(x = paste0(d, " times"))
Copy link
Member

Choose a reason for hiding this comment

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

All of these minor fixes definitely imply that the old behaviour was silently obscuring a lot of problems.

R/rows.R Outdated Show resolved Hide resolved
R/tidyeval-across.R Outdated Show resolved Hide resolved
R/tidyeval-across.R Show resolved Hide resolved
R/tidyeval.R Outdated Show resolved Hide resolved
R/tidyeval-across.R Outdated Show resolved Hide resolved
tests/testthat/_snaps/verb-group_by.md Show resolved Hide resolved
tests/testthat/test-tidyeval.R Show resolved Hide resolved

for (j in seq_along(quos)) {
dot_name <- names(quos)[[j]]
summarise_bind_error1(error_env, dot_name)
Copy link
Collaborator Author

Choose a reason for hiding this comment

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

It is a bit of a hack but works nicely.
When the next dot is evaluated partial_eval_sym() searches for the variable in cur_data. As we just removed it, it is evaluated in the environment of the dot. By lazily binding an error to the variable in the environment we get the old behaviour - even slightly improved.

@mgirlich mgirlich merged commit 54107c4 into main Jul 31, 2022
@mgirlich mgirlich deleted the improved-error-messages branch July 31, 2022 08:10
ateucher added a commit to bcgov/bcdata that referenced this pull request Sep 9, 2022
This parsing of calls to identify bits that should be evaluated locally was quite fragile and only possible due to previously loose tolerances in dbplyr::partial_eval. With the change in dbplyr 2.2.2 (tidyverse/dbplyr#907) this loophole is closed, and exposes the fact that the identification of parts of the call that should be evaluated locally didn't always work. With this change users will now need to explicitly use `local()` to wrap function calls that should be evaluated locally before being translated to CQL and sent to the wfs server.
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.

Should strict_sql also error on unknown variables? Bug: can use distinct() on removed variable
2 participants