-
Notifications
You must be signed in to change notification settings - Fork 4k
GH-47167: [C++][Dev] Update clang-format dependency #47168
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
base: main
Are you sure you want to change the base?
Conversation
|
|
|
The Python failures are unrelated, I just merged the fix on main, if you rebase they will go away. |
|
Without it, that sends back |
|
Supporting old pre-commit (system pre-commit) is for new contributors on Ubuntu 22.04. If we have enough documentation (or something) for new contributors, we can require more newer pre-commit (and identify). |
|
|
|
apache/arrow has many language implementations. We can't assume that all new contributors are familiar with Python, conda and/or pip. For example, this thread #46686 (comment) considered about new R contributors. |
|
A possible direction to help both could be to leverage pixi. I'm using it locally and in many other projects (C++/Rust/Python): it combines conda dependencies and run commands in managed environments. Once installed (which is fairly easy IMHO), a one command I'm not claiming it could handle all Arrow development cases, but it certainly make for a great on-boarding. |
|
I don't have a strong opinion how to care about new contributors. I don't object the Pixi (or pip) approach if we have the official document how to prepare pre-commit with Pixi or something. It may be better that we start a discussion thread on |
|
I think it becomes more reasonable now to move forward this PR, since:
I've made a commit in this PR with an update to the clang-format version change from v20 to a less aggressive v18, and related code formatting. @kou @pitrou @AntoinePrv what do you think? Thanks. [1] arrow/.github/workflows/cpp.yml Line 84 in 7fe39d4
[2] Line 60 in 7fe39d4
[3] arrow/cpp/src/arrow/compute/kernels/hash_aggregate.cc Lines 283 to 285 in 3fb84ff
[4] llvm/llvm-project#56283 [5] https://releases.llvm.org/15.0.0/tools/clang/docs/ReleaseNotes.html#clang-format |
That's fine with me on the principle. The lint CI job is failing, though. |
|
How about requiring (recommending?) Ubuntu 24.04 or later (not 22.04) for development in our documentation? |
Rationale for this change
Update clang-format to better match modern IDEs.
What changes are included in this PR?
Are these changes tested?
Yes
Are there any user-facing changes?
No