Refactor capture of arguments

njtierney / naniar

Tidy data structures, summaries, and visualisations for missing data

http://naniar.njtierney.com/

Other

652 stars 53 forks source link

Refactor capture of arguments #218

Closed lionel- closed 6 years ago

lionel- commented 6 years ago

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- commented 6 years ago

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 commented 6 years ago

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- commented 6 years ago

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 commented 6 years ago

Hiya @lionel- ,

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

lionel- commented 6 years ago

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 commented 6 years ago

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?