ARROW-17084: [R] Install the package before linting #13620
Conversation
|
@jonkeane & @nealrichardson would you mind having a look? (when you get a chance) |
There was a problem hiding this comment.
I'm fine to merge this as-is, though is there a CI job we expect will fail or should now pass with these changes?
We don't absolutely have to do the dance of introducing a linting error, demonstrating it fails, and then demonstrating it works but it might be nice to confirm (since presumably we were failing or accidentally not linting at all before? Apologies if this is discussed elsewhere, I looked through the description here + ticket and couldn't find a precipitating cause of finding this bug(?))
|
@jonkeane Thanks for your review. Hopefully this gives more details (and answers the questions):
|
|
Interesting, thanks for that additional context. We should merge this fix regardless (since it's the recommended way of running lintr!), but I suspect that something else might have happened with the seemingly off files here. I was curios, so went looking and turns out, the offending whitespace line was introduced in #13610 which did get properly flagged: https://github.com/apache/arrow/runs/7349390401?check_suite_focus=true so at least that did work at the time. Anyway, like I said, I'll merge this since it's the right way to be running lintr according to the developers |
|
I see what you mean. I guess it depends on the point of view. If you look at it as the state of the package pre-merge (my branch) then the lint is a false-positive (which is AFAICT should be the case). If you look at it as the state of the package post-merge (the master) then the picked-up lint was not a false-positive. As you say, regardless of the POV, the authors' recommended workflow is to install the package before linting. If my understanding is correct, it points to a way we could test this. Introduce a lint in a new file in master. With the old workflow any subsequent PR should pick-up this lint (even if the affected file is not touched), while with the new CI workflow, the lint in master should not be picked-up (if the file isn't present in the branch). |
|
Benchmark runs are scheduled for baseline = 48e2780 and contender = 51eb3c8. 51eb3c8 is a master commit associated with this PR. Results will be available as each benchmark for each run completes. |
The package should be installed before running
lintr::ling_package()orlintr::expect_lint_free()(our case), otherwise we could encounter some false positives.See r-lib/lintr#352 (comment) and r-lib/lintr#406 (comment)