You are viewing a plain text version of this content. The canonical link for it is here.
Posted to github@arrow.apache.org by "thisisnic (via GitHub)" <gi...@apache.org> on 2025/08/25 11:31:16 UTC
[PR] Draft: GH-46592: [CI][Dev][R] Add Air to pre-commit [arrow]
thisisnic opened a new pull request, #47423:
URL: https://github.com/apache/arrow/pull/47423
### Rationale for this change
### What changes are included in this PR?
### Are these changes tested?
### Are there any user-facing changes?
TODO:
- wait for action to be approved by infra
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-46592: [CI][Dev][R] Add Air to pre-commit [arrow]
Posted by "thisisnic (via GitHub)" <gi...@apache.org>.
thisisnic commented on PR #47423:
URL: https://github.com/apache/arrow/pull/47423#issuecomment-3220343134
@jonkeane I'm thinking we bring in air and drop styler, but perhaps still need lintr - what do you think?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-46592: [CI][Dev][R] Add Air to pre-commit [arrow]
Posted by "jonkeane (via GitHub)" <gi...@apache.org>.
jonkeane commented on PR #47423:
URL: https://github.com/apache/arrow/pull/47423#issuecomment-3221577318
> > > we end up losing lintr's object_length_linter and cyclocomp_linter, but it seems reasonable to enforce these conventions via code review anyway.
> >
> > Agreed. We had already been setting the cyclocomp pretty high anyway, I'm not sure there's much value in it. I'm a little surprised by the object length linter going away?
>
> I had a bit more of a read and I think the nuance is that a styler/formatter and a linter are different things, and so air replaces styler not lintr, which I guess makes sense as there are some things lintr can pick up that styler can't fix, we just don't end up encountering them as much on arrow.
Ah, yep yep that's right ok cool! I'm still fine 🚮 the cyclocomp lintr even if we keep the others that will enforce (for folks who haven't installed the pre-commit hook or who override it, etc.) our styling.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-46592: [CI][Dev][R] Add Air to pre-commit [arrow]
Posted by "thisisnic (via GitHub)" <gi...@apache.org>.
thisisnic commented on PR #47423:
URL: https://github.com/apache/arrow/pull/47423#issuecomment-3377162317
Cheers, I will both add a comment and then push the air-formatted changes to the branch before merging!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-46592: [CI][Dev][R] Add Air to pre-commit [arrow]
Posted by "conbench-apache-arrow[bot] (via GitHub)" <gi...@apache.org>.
conbench-apache-arrow[bot] commented on PR #47423:
URL: https://github.com/apache/arrow/pull/47423#issuecomment-3447891923
After merging your PR, Conbench analyzed the 4 benchmarking runs that have been run so far on merge-commit 308b007bc82da3e5e46c5abd72926a0b0ec5a3bc.
There were no benchmark performance regressions. 🎉
The [full Conbench report](https://github.com/apache/arrow/runs/53670753415) has more details. It also includes information about 11 possible false positives for unstable benchmarks that are known to sometimes produce them.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-46592: [CI][Dev][R] Add Air to pre-commit [arrow]
Posted by "jonkeane (via GitHub)" <gi...@apache.org>.
jonkeane commented on code in PR #47423:
URL: https://github.com/apache/arrow/pull/47423#discussion_r2410672671
##########
r/air.toml:
##########
@@ -0,0 +1,20 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+[format]
+line-width = 120
+exclude = ["R/arrowExports.R", "R/dplyr-funcs-doc.R", "data-raw/codegen.R"]
Review Comment:
This might be covered elsewhere, but do we have a note about why we don't style `R/dplyr-funcs-doc.R`? The other two make sense (since they are generated / do generation) but the middle one surprises me a little. Maybe add a comment here explaining?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-46592: [CI][Dev][R] Add Air to pre-commit [arrow]
Posted by "jonkeane (via GitHub)" <gi...@apache.org>.
jonkeane commented on PR #47423:
URL: https://github.com/apache/arrow/pull/47423#issuecomment-3220406571
But generally, yes I'm super supportive of this!
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-46592: [CI][Dev][R] Add Air to pre-commit [arrow]
Posted by "jonkeane (via GitHub)" <gi...@apache.org>.
jonkeane commented on PR #47423:
URL: https://github.com/apache/arrow/pull/47423#issuecomment-3220403847
> we end up losing lintr's object_length_linter and cyclocomp_linter, but it seems reasonable to enforce these conventions via code review anyway.
Agreed. We had already been setting the cyclocomp pretty high anyway, I'm not sure there's much value in it. I'm a little surprised by the object length linter going away?
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-46592: [CI][Dev][R] Add Air to pre-commit [arrow]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #47423:
URL: https://github.com/apache/arrow/pull/47423#issuecomment-3220025208
:warning: GitHub issue #46592 **has been automatically assigned in GitHub** to PR creator.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-46592: [CI][Dev][R] Add Air to pre-commit [arrow]
Posted by "thisisnic (via GitHub)" <gi...@apache.org>.
thisisnic commented on PR #47423:
URL: https://github.com/apache/arrow/pull/47423#issuecomment-3220152407
I'm looking at differences between `air` and `lintr`, and if we just use `air`, we end up losing lintr's `object_length_linter` and `cyclocomp_linter`, but it seems reasonable to enforce these conventions via code review anyway.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] Draft: GH-46592: [CI][Dev][R] Add Air to pre-commit [arrow]
Posted by "github-actions[bot] (via GitHub)" <gi...@apache.org>.
github-actions[bot] commented on PR #47423:
URL: https://github.com/apache/arrow/pull/47423#issuecomment-3219907773
<!--
Licensed to the Apache Software Foundation (ASF) under one
or more contributor license agreements. See the NOTICE file
distributed with this work for additional information
regarding copyright ownership. The ASF licenses this file
to you under the Apache License, Version 2.0 (the
"License"); you may not use this file except in compliance
with the License. You may obtain a copy of the License at
http://www.apache.org/licenses/LICENSE-2.0
Unless required by applicable law or agreed to in writing,
software distributed under the License is distributed on an
"AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
KIND, either express or implied. See the License for the
specific language governing permissions and limitations
under the License.
-->
Thanks for opening a pull request!
If this is not a [minor PR](https://github.com/apache/arrow/blob/main/CONTRIBUTING.md#Minor-Fixes). Could you open an issue for this pull request on GitHub? https://github.com/apache/arrow/issues/new/choose
Opening GitHub issues ahead of time contributes to the [Openness](http://theapacheway.com/open/#:~:text=Openness%20allows%20new%20users%20the,must%20happen%20in%20the%20open.) of the Apache Arrow project.
Then could you also rename the pull request title in the following format?
GH-${GITHUB_ISSUE_ID}: [${COMPONENT}] ${SUMMARY}
or
MINOR: [${COMPONENT}] ${SUMMARY}
See also:
* [Other pull requests](https://github.com/apache/arrow/pulls/)
* [Contribution Guidelines - Contributing Overview](https://arrow.apache.org/docs/developers/overview.html)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-46592: [CI][Dev][R] Add Air to pre-commit [arrow]
Posted by "jonkeane (via GitHub)" <gi...@apache.org>.
jonkeane commented on PR #47423:
URL: https://github.com/apache/arrow/pull/47423#issuecomment-3444445134
> @jonkeane Went down a rabbit hole with this one, but it appears that right now, air doesn't support .Rmd files, so our vignettes. My take is that we update these so infrequently and air is so fast, that it's worth the omission, but keen to get your take before I fix+merge this without styling on our vignettes.
Yeah, I'm fine with that too. For my own knowledge: does it work with qmds? Is this a sign that we should transition to those (separately, outside of this PR!)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-46592: [CI][Dev][R] Add Air to pre-commit [arrow]
Posted by "thisisnic (via GitHub)" <gi...@apache.org>.
thisisnic commented on code in PR #47423:
URL: https://github.com/apache/arrow/pull/47423#discussion_r2410866374
##########
r/air.toml:
##########
@@ -0,0 +1,20 @@
+# Licensed to the Apache Software Foundation (ASF) under one
+# or more contributor license agreements. See the NOTICE file
+# distributed with this work for additional information
+# regarding copyright ownership. The ASF licenses this file
+# to you under the Apache License, Version 2.0 (the
+# "License"); you may not use this file except in compliance
+# with the License. You may obtain a copy of the License at
+#
+# http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing,
+# software distributed under the License is distributed on an
+# "AS IS" BASIS, WITHOUT WARRANTIES OR CONDITIONS OF ANY
+# KIND, either express or implied. See the License for the
+# specific language governing permissions and limitations
+# under the License.
+
+[format]
+line-width = 120
+exclude = ["R/arrowExports.R", "R/dplyr-funcs-doc.R", "data-raw/codegen.R"]
Review Comment:
Will do! (It's another generated one, the name just looks like our code files)
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-46592: [CI][Dev][R] Add Air to pre-commit [arrow]
Posted by "thisisnic (via GitHub)" <gi...@apache.org>.
thisisnic merged PR #47423:
URL: https://github.com/apache/arrow/pull/47423
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-46592: [CI][Dev][R] Add Air to pre-commit [arrow]
Posted by "thisisnic (via GitHub)" <gi...@apache.org>.
thisisnic commented on PR #47423:
URL: https://github.com/apache/arrow/pull/47423#issuecomment-3446809833
> > @jonkeane Went down a rabbit hole with this one, but it appears that right now, air doesn't support .Rmd files, so our vignettes. My take is that we update these so infrequently and air is so fast, that it's worth the omission, but keen to get your take before I fix+merge this without styling on our vignettes.
>
> Yeah, I'm fine with that too. For my own knowledge: does it work with qmds? Is this a sign that we should transition to those (separately, outside of this PR!)?
My understanding is that it works with qmds inside Positron but not via the CLI: https://github.com/posit-dev/air/issues/221
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-46592: [CI][Dev][R] Add Air to pre-commit [arrow]
Posted by "thisisnic (via GitHub)" <gi...@apache.org>.
thisisnic commented on PR #47423:
URL: https://github.com/apache/arrow/pull/47423#issuecomment-3411293114
@jonkeane Went down a rabbit hole with this one, but it appears that right now, air doesn't support .Rmd files, so our vignettes. My take is that we update these so infrequently and air is so fast, that it's worth the omission, but keen to get your take before I fix+merge this without styling on our vignettes.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org
Re: [PR] GH-46592: [CI][Dev][R] Add Air to pre-commit [arrow]
Posted by "thisisnic (via GitHub)" <gi...@apache.org>.
thisisnic commented on PR #47423:
URL: https://github.com/apache/arrow/pull/47423#issuecomment-3221070972
> > we end up losing lintr's object_length_linter and cyclocomp_linter, but it seems reasonable to enforce these conventions via code review anyway.
>
> Agreed. We had already been setting the cyclocomp pretty high anyway, I'm not sure there's much value in it. I'm a little surprised by the object length linter going away?
I had a bit more of a read and I think the nuance is that a styler/formatter and a linter are different things, and so air replaces styler not lintr, which I guess makes sense as there are some things lintr can pick up that styler can't fix, we just don't end up encountering them as much on arrow.
--
This is an automated message from the Apache Git Service.
To respond to the message, please log on to GitHub and use the
URL above to go to the specific comment.
To unsubscribe, e-mail: github-unsubscribe@arrow.apache.org
For queries about this service, please contact Infrastructure at:
users@infra.apache.org