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

Refactor capture of arguments #218

Merged
merged 3 commits into from Oct 22, 2018

Conversation

Projects
None yet
2 participants
@lionel-
Copy link
Contributor

lionel- commented Oct 6, 2018

  • Pass ... directly to other functions rather than quote and splice.

  • Use as_string() rather than expr_text() to cast symbols to strings. The latter is a multi-line deparser for arbitrary expressions. It might add backticks to deparsed symbols and allow unwanted input types like complex expressions.

  • Use ensyms() before coercing to strings. This guarantees only symbols can be passed by user. quos() allows stuff like starts_with(), but the code generally assumes symbols, not calls.

  • Remove bare_to_chr() as part of this refactoring. It was making the assumption that exprs() unwraps quosures, which was a bug in rlang. This causes a revdep failure for the upcoming rlang 0.3.0.

lionel- added some commits Oct 6, 2018

Use as_string() rather than expr_text()
The latter is not appropriate here because it's a multi-line deparser
for arbitrary expressions. Symbols should be cast to strings with
`as.character()` or `as_string()`.
Use ensyms() rather than quos() to capture variables
The latter is problematic because it allows arbitrary expressions.
These variables are forwarded to select(), so potential expressions
are `starts_with()`, `one_of()`, etc.  The naniar code generally
assumes that only symbols are passed in dots. `ensyms()` is a way of
ensuring the input types.

@lionel- lionel- force-pushed the lionel-:fix-capture branch from 8344214 to 3e0552b Oct 6, 2018

@lionel-

This comment has been minimized.

Copy link
Contributor

lionel- commented Oct 6, 2018

hmm, I think we can do better by calling into tidyselect::vars_select(). We should be able to support selection calls. I'll follow up with more commits.

@njtierney

This comment has been minimized.

Copy link
Owner

njtierney commented Oct 8, 2018

Thank you very much for this! :)

Is there a deadline for getting this fixed and onto CRAN so that revdep for rlang passes? Happy to help move things along with a patch release.

@lionel-

This comment has been minimized.

Copy link
Contributor

lionel- commented Oct 8, 2018

I think I'll revert the changes to expr_name() and quo_name() for now so they don't cause too much trouble for this release. In that case I think naniar won't need any urgent changes.

@njtierney

This comment has been minimized.

Copy link
Owner

njtierney commented Oct 19, 2018

Hiya @lionel- ,

I just wanted to check if I should merge this and submit a patch release for naniar?

@lionel-

This comment has been minimized.

Copy link
Contributor

lionel- commented Oct 19, 2018

Hmm yes good idea, I was busy lately sorry. IIRC naniar is not compatible with rlang devel because it assumes exprs() flattens quosures and this PR should fix it. As soon as I have time I'll work on that tidyselect PR!

@njtierney

This comment has been minimized.

Copy link
Owner

njtierney commented Oct 22, 2018

OK great, I'll merge this now, thank you again! :)

It sounds like there will be some changes to tidyselect in the future WRT quasiquotation?

@njtierney njtierney merged commit 1cb2493 into njtierney:master Oct 22, 2018

1 of 2 checks passed

continuous-integration/travis-ci/pr The Travis CI build could not complete due to an error
Details
continuous-integration/appveyor/pr AppVeyor build succeeded
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment