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

Update test for unwind::protect error in vroom_errors::warn_for_errors() #452

Merged
merged 5 commits into from
Aug 22, 2022

Conversation

sbearrows
Copy link
Contributor

Fixes #445

This might be a bit redundant/circular, but I'm going to relink @DavisVaughan issue post about this because I found it really useful for refreshing my memory on the problem

r-lib/cpp11#274

@sbearrows sbearrows requested a review from jennybc July 18, 2022 22:08
@sbearrows
Copy link
Contributor Author

@jennybc This is ready for you to review

Copy link
Member

@jennybc jennybc left a comment

Choose a reason for hiding this comment

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

The update to the test looks good.

I'm pre-approving, but am also suggesting a change to this load-bearing comment, which is a key piece of documentation for something quite subtle.

Comment on lines 162 to 163
# R's internal C code (which doesn't catch C++ exceptions), `vroom::warn_for_errors()` warns
# with base R's machinery rather than cpp11's
Copy link
Member

Choose a reason for hiding this comment

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

vroom::warn_for_errors() warns with base R's machinery rather than cpp11's

This review prompted me to read 👆 for the first time in a long time and then compare it to the source for warn_for_error():

vroom/src/vroom_errors.h

Lines 85 to 103 in f21f314

void warn_for_errors() const {
if (!have_warned_ && rows_.size() > 0) {
have_warned_ = true;
// it is intentional that we aren't using cpp11::package
// https://github.com/tidyverse/vroom/commit/984a3e5e37e124feacfec3d184dbeb02eb1145c4
static auto cli_warn = Rf_findFun(
Rf_install("cli_warn"),
Rf_findVarInFrame(R_NamespaceRegistry, Rf_install("cli")));
cpp11::strings bullets({
"w"_nm = "One or more parsing issues, call {.fun problems} on your data frame for details, e.g.:",
" "_nm = "dat <- vroom(...)",
" "_nm = "problems(dat)"});
cpp11::sexp cli_warn_call = Rf_lang3(
cli_warn,
bullets,
Rf_mkString("vroom_parse_issue"));
Rf_eval(cli_warn_call, R_EmptyEnv);
}
}

With fresh eyes, I feel like we can refine this to be even more precise. I'd say we're warning with cli and we're using base R machinery to access the relevant R function, as opposed to accessing it via cpp11 way.

@sbearrows sbearrows merged commit 339073c into main Aug 22, 2022
@sbearrows sbearrows deleted the update-test branch August 29, 2022 20:03
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.

Update test for problems() unwind::protect error
3 participants