-
Notifications
You must be signed in to change notification settings - Fork 4k
ARROW-13739: [R] Support dplyr::count() and tally() #11306
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
Conversation
|
|
|
There's also a skipped test-dataset-something.R test about count, you should unskip it in this PR |
|
The failing test here fails once per session with something like but on subsequent runs in the same session, it's fine and passes. I'm a bit unsure of what to do here as I looked for the line of code which triggers it but see no Is this something we've seen before @nealrichardson ? |
3c2899f to
b551527
Compare
"This warning is displayed once per session" is why it doesn't happen the next time. I've seen it before but not lately. I'll have to pull and take a look. |
|
Turns out we're getting quosures inside of quosures still. I'll suggest another patch, which might help for any others who are building NSE wrappers, and if that doesn't work, we can go back to the |
Co-authored-by: Neal Richardson <neal.p.richardson@gmail.com>
Co-authored-by: Neal Richardson <neal.p.richardson@gmail.com>
Co-authored-by: Neal Richardson <neal.p.richardson@gmail.com>
Co-authored-by: Neal Richardson <neal.p.richardson@gmail.com>
2470920 to
23dd3df
Compare
|
I've fixed the tests, 🤞 we get a passing build now |
r/R/util.R
Outdated
|
|
||
| is_function <- function(expr, name) { | ||
| # We could have a quosure here if we have an expression like | ||
| # `sum({{ var }})` |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@jonkeane How do I make this lint message go away?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
There are a few options: having more words in front of the code or # nolint at the end. We can also turn this one particular lintr off (or it's possible there are settings for it that makes it less sensitive) by adding commented_code_linter = NULL to https://github.com/apache/arrow/blob/master/r/.lintr#L25
I've been meaning to send a PR to lintr to make backticks like this defuse commented code — though their track record of merging these has been spotty so I haven't made it a priority.
nealrichardson
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Just need to fix the style/lint stuff but then LGTM!
Co-authored-by: Neal Richardson <neal.p.richardson@gmail.com>
No description provided.