fix: Add the missing brackets and keep the class member name consistency#479
fix: Add the missing brackets and keep the class member name consistency#479leerho merged 1 commit intoapache:masterfrom
Conversation
|
Hi @leerho, could you please review this PR? I'd appreciate your feedback. |
|
I am not sure I support changing short one-line if statements like this if (condition) return 0; to if (condition) { We did not establish any style requirements, and we use one-liners throughout the C++ library. So why should we change the approach in this particular class? There is no harm, of course, but this style is a bit more verbose. I would not mind switching to this style if the community wants. |
I appreciate your perspective. While this individual style change might seem minor, I believe we should think bigger: implementing a I'd like to propose we adopt a Does this sound reasonable to you? We could start by researching configurations from similar Apache projects. |
|
I tend to favor this idea, because I really like clean, readable, and robust code. In the ds-Java library we use Checkstyle when possible and a checkstyle.xml config file is checked in. (E.g., Checkstyle is not compatible with Java 25 yet) However, I would only agree to this if
It would be ideal if we had something similar for all the languages 🙂. |
Thanks for the explanation — I’m aligned with this approach. I’m willing to volunteer to go through the existing C++ code, carefully review the style discrepancies, and apply fixes without relying on an automatic formatter. Once the team agrees on the Checkstyle configuration, I can start working on this. For reference, here is the Apache Arrow .clang-tidy configuration: If the team agrees on adopting this configuration (or a close variant of it), I can address the formatting issues incrementally, algorithm by algorithm. |
|
While I personally prefer brief one-line style, I don't mind adopting a stricter approach with mandatory curly brackets if we consistently use it throughout the whole code in C++. |
|
I agree with the proposed .clang-tidy configuration.
…On Mon, Feb 9, 2026 at 11:51 AM Alexander Saydakov ***@***.***> wrote:
***@***.**** approved this pull request.
—
Reply to this email directly, view it on GitHub
<#479 (review)>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/ADCXRQS4FGFMJXRRNGCD2OD4LDQMRAVCNFSM6AAAAACTQK2ON6VHI2DSMVQWIX3LMV43YUDVNRWFEZLROVSXG5CSMV3GSZLXHMZTONZUHE4TIOJZHA>
.
You are receiving this because you were mentioned.Message ID:
***@***.***>
|
Pull Request Test Coverage Report for Build 22037164944Details
💛 - Coveralls |
leerho
left a comment
There was a problem hiding this comment.
I’ll approve but this still could have been on one line: if (…) {…}.
Ok, I will fix it. |
|
I have updated the code to support one line statement. |
use clang-tidy fix the warning