diff mbox series

[1/3] clang-format: don't enforce the column limit

Message ID CAOLa=ZS+naxOzJUkLLOZk++WVZ2dt3eQq9VmW+G-5O1ZLgggUA@mail.gmail.com (mailing list archive)
State New
Headers show
Series [1/3] clang-format: don't enforce the column limit | expand

Commit Message

karthik nayak Oct. 9, 2024, 12:55 p.m. UTC
The current value for the column limit is set to 80. While this is as
expected, we often prefer readability over this strict limit. This means
it is common to find code which extends over 80 characters. So let's
change the column limit to be 0 instead. This ensures that the formatter
doesn't complain about code strictly not following the column limit.

Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
---
 .clang-format | 6 +++++-
 1 file changed, 5 insertions(+), 1 deletion(-)

Comments

Justin Tobler Oct. 9, 2024, 3:45 p.m. UTC | #1
On 24/10/09 05:55AM, Karthik Nayak wrote:
> The current value for the column limit is set to 80. While this is as
> expected, we often prefer readability over this strict limit. This means
> it is common to find code which extends over 80 characters. So let's
> change the column limit to be 0 instead. This ensures that the formatter
> doesn't complain about code strictly not following the column limit.

The column limit does lead to quite a few false positives. At the same
time though, in some ways having a tool point out all the instances it
occurs does make it easier to review if any should be addressed.

If the goal is to have a CI job that we generally expect to pass, then
it makes sense to remove it. I don't feel super strongly either way.

-Justin
Junio C Hamano Oct. 9, 2024, 10:32 p.m. UTC | #2
Justin Tobler <jltobler@gmail.com> writes:

> On 24/10/09 05:55AM, Karthik Nayak wrote:
>> The current value for the column limit is set to 80. While this is as
>> expected, we often prefer readability over this strict limit. This means
>> it is common to find code which extends over 80 characters. So let's
>> change the column limit to be 0 instead. This ensures that the formatter
>> doesn't complain about code strictly not following the column limit.
>
> The column limit does lead to quite a few false positives. At the same
> time though, in some ways having a tool point out all the instances it
> occurs does make it easier to review if any should be addressed.
>
> If the goal is to have a CI job that we generally expect to pass, then
> it makes sense to remove it. I don't feel super strongly either way.

Is it possible for gatekeeper jobs to complain only on newly added
violations?  Then it is fine to have a limit with a bit of slack,
say like 96 columns (with 16-column readability slack).
karthik nayak Oct. 10, 2024, 4:48 p.m. UTC | #3
Junio C Hamano <gitster@pobox.com> writes:

> Justin Tobler <jltobler@gmail.com> writes:
>
>> On 24/10/09 05:55AM, Karthik Nayak wrote:
>>> The current value for the column limit is set to 80. While this is as
>>> expected, we often prefer readability over this strict limit. This means
>>> it is common to find code which extends over 80 characters. So let's
>>> change the column limit to be 0 instead. This ensures that the formatter
>>> doesn't complain about code strictly not following the column limit.
>>
>> The column limit does lead to quite a few false positives. At the same
>> time though, in some ways having a tool point out all the instances it
>> occurs does make it easier to review if any should be addressed.
>>
>> If the goal is to have a CI job that we generally expect to pass, then
>> it makes sense to remove it. I don't feel super strongly either way.
> Is it possible for gatekeeper jobs to complain only on newly added
> violations?

The CI job is indeed only checking the newly added code. We do this
using 'git clang-format' which takes in the basecommit as a param. This
is in 'ci/run-style-check.sh'.

> Then it is fine to have a limit with a bit of slack,
> say like 96 columns (with 16-column readability slack).

This is a good idea. Let me add change this in the next version.
diff mbox series

Patch

diff --git a/.clang-format b/.clang-format
index 41969eca4b..38910a3a53 100644
--- a/.clang-format
+++ b/.clang-format
@@ -12,7 +12,11 @@  UseTab: Always
 TabWidth: 8
 IndentWidth: 8
 ContinuationIndentWidth: 8
-ColumnLimit: 80
+
+# While we recommend keeping column limit to 80, we don't want to
+# enforce it as we generally are more lenient with this rule and
+# prefer to prioritize readability.
+ColumnLimit: 0

 # C Language specifics
 Language: Cpp