Skip to content

Conversation

@thisisnic
Copy link
Member

No description provided.

@github-actions
Copy link

github-actions bot commented Oct 4, 2021

@github-actions
Copy link

github-actions bot commented Oct 4, 2021

⚠️ Ticket has not been started in JIRA, please click 'Start Progress'.

@nealrichardson
Copy link
Member

There's also a skipped test-dataset-something.R test about count, you should unskip it in this PR

@thisisnic
Copy link
Member Author

thisisnic commented Oct 6, 2021

The failing test here fails once per session with something like

Failure (test-dplyr-count-tally.R:43:3): count/tally with wt and grouped data
`via_batch <- rlang::eval_tidy(expr, rlang::new_data_mask(rlang::env(input = record_batch(tbl))))` threw an unexpected warning.
Message: Subsetting quosures with `[[` is deprecated as of rlang 0.4.0
Please use `quo_get_expr()` instead.
This warning is displayed once per session.
Class:   deprecatedWarning/warning/condition

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 [[ there.

Is this something we've seen before @nealrichardson ?

@thisisnic thisisnic force-pushed the ARROW-13739_dplyr_count_tally branch from 3c2899f to b551527 Compare October 6, 2021 14:23
@nealrichardson
Copy link
Member

The failing test here fails once per session with something like

Failure (test-dplyr-count-tally.R:43:3): count/tally with wt and grouped data
`via_batch <- rlang::eval_tidy(expr, rlang::new_data_mask(rlang::env(input = record_batch(tbl))))` threw an unexpected warning.
Message: Subsetting quosures with `[[` is deprecated as of rlang 0.4.0
Please use `quo_get_expr()` instead.
This warning is displayed once per session.
Class:   deprecatedWarning/warning/condition

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 [[ there.

Is this something we've seen before @nealrichardson ?

"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.

@nealrichardson
Copy link
Member

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 expr() building that dplyr does.

@nealrichardson nealrichardson force-pushed the ARROW-13739_dplyr_count_tally branch from 2470920 to 23dd3df Compare October 6, 2021 21:28
@nealrichardson
Copy link
Member

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 }})`
Copy link
Member

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?

Copy link
Member

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.

Copy link
Member

@nealrichardson nealrichardson left a 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>
@romainfrancois romainfrancois self-requested a review October 7, 2021 16:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants