GH-46798: [CI][Dev] Add support for pre-commit 2.17.0 #46799
Conversation
|
|
assignUser
left a comment
assignUser
left a comment
There was a problem hiding this comment.
I am -0.5 on this. With pre-commit being a normal python package that can be easily installed I don't think we should limit ourselves to OS provided versions.
Though in this case I doubt there are many differences between the two versions? If some other hook needs a newer version we can always reconsider.
I can understand your opinion but there are developers/contributors who aren't familiar with Python packages because this repository has many language implementations. So I want to reduce developers/contributors costs to prepare their development environment.
I'm OK with this approach. We may require more recent pre-commit when we need it before Ubuntu 22.04 reaches EOL. |
I'm neutral on what approach we take but we absolutely should make sure we're not introducing additional friction for developers. If we choose not to support the older version, we should at least be very explicit about installation method in our docs then - i.e. that you must install the package using pip not apt. The error message from pre-commit in the case of an older version doesn't make it obvious why it's not working. Took a good chunk of time and frustration (until dumping the error into an LLM worked) to solve it when I ran into this.
The |
|
OK. Let's use this approach (developers/contributors can use their common ways to install pre-commit. e.g. |
|
@thisisnic yeah the error message is not great. The shfmt hook makes use of a feature only introduced in newer version so it sets a lower limit. I don't understand why the error doesn't say that... Unless there is a feature we want to use in newer version/hooks I'm fine with making this apt compatible! |
…6799) ### Rationale for this change Ubuntu 22.04 ships pre-commit 2.17.0. So we should support pre-commit 2.17.0 for easy to develop. ### What changes are included in this PR? * Use a bit older shfmt pre-commit configuration for pre-commit 2.17.0 * Use Ubuntu 22.04 on CI to ensure working on Ubuntu 22.04 ### Are these changes tested? Yes. ### Are there any user-facing changes? No. * GitHub Issue: apache#46798 Authored-by: Sutou Kouhei <[email protected]> Signed-off-by: Sutou Kouhei <[email protected]>
|
After merging your PR, Conbench analyzed the 0 benchmarking runs that have been run so far on merge-commit 69f55f0. None of the specified runs were found on the Conbench server. The full Conbench report has more details. |
3 participants
Rationale for this change
Ubuntu 22.04 ships pre-commit 2.17.0. So we should support pre-commit 2.17.0 for easy to develop.
What changes are included in this PR?
Are these changes tested?
Yes.
Are there any user-facing changes?
No.