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

Align partialeval with dtplyr #766

Merged
merged 36 commits into from
Mar 2, 2022
Merged

Align partialeval with dtplyr #766

merged 36 commits into from
Mar 2, 2022

Conversation

mgirlich
Copy link
Collaborator

Fixes #760 and fixes #669.

This aligns the partial eval file with dtplyr so that fixes and improvements can be transferred more easily from one to the other. Most functions are now basically identical. Still different:

  • some function names. Should we align names? If so which system do you prefer?
    • partial_eval() vs dt_squash()
    • partial_eval_dots() vs capture_dots()
    • partial_eval_call() vs dt_squash_call()
    • partial_eval_across() vs dt_squash_across()
  • dbplyr functions take a vars argument but it might make more sense to pass data (e.g. because across() does not compute on group columns). Changing this would require changing the interface of the exported function partial_eval() but might still be worth it.

@mgirlich mgirlich mentioned this pull request Jan 21, 2022
Copy link
Member

@hadley hadley left a comment

Choose a reason for hiding this comment

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

I like the idea of consistently using partial_eval_ as prefix.

R/tidyeval.R Outdated Show resolved Hide resolved
R/tidyeval.R Outdated Show resolved Hide resolved
tests/testthat/test-tidyeval-across.R Show resolved Hide resolved
tests/testthat/test-tidyeval-across.R Outdated Show resolved Hide resolved
@hadley
Copy link
Member

hadley commented Feb 25, 2022

Thanks for working on this!

@mgirlich mgirlich merged commit 8a1b7b6 into main Mar 2, 2022
@mgirlich mgirlich deleted the across-dtplyr-compat branch March 2, 2022 15: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.

Test across() better dbplyr across(...) doesn't work with .cols = character()
2 participants