[Gluster-devel] Automatic clang-format for GitHub PRs

[Amar Tumballi]

On Thu, 11 Feb, 2021, 9:19 pm Xavi Hernandez, <xhernandez at redhat.com> wrote:

> On Wed, Feb 10, 2021 at 1:33 PM Amar Tumballi <amar at kadalu.io> wrote:
>
>>
>>
>> On Wed, Feb 10, 2021 at 3:29 PM Xavi Hernandez <xhernandez at redhat.com>
>> wrote:
>>
>>> Hi all,
>>>
>>> I'm wondering if enforcing clang-format for all patches is a good idea...
>>>
>>> I've recently seen patches where clang-format is doing changes on parts
>>> of the code that have not been touched by the patch. Given that all files
>>> were already formatted by clang-format long ago, this shouldn't happen.
>>>
>>> This means that as the clang-format version evolves, the formatting with
>>> the same configuration is not the same. This introduces unnecessary noise
>>> to the file history that I think it should be avoided.
>>>
>>> Additionally, I've also seen some cases where some constructs are
>>> reformatted in an uglier or less clear way. I think it's very hard to come
>>> up with a set of rules that formats everything in the best possible way.
>>>
>>> For all these reasons, I would say we shouldn't enforce clang-format to
>>> accept a PR. I think it's a good test to run to catch some clear formatting
>>> issues, but it shouldn't vote for patch acceptance.
>>>
>>> What do you think ?
>>>
>>>
>> One thing I have noticed is, as long as some test is 'skipped', no one
>> bothers to check. It would be great if the whole diff (in case of failure)
>> is posted as a comment, so we can consider that while merging. I would
>> request one to invest time on posting the failure message as a comment back
>> into issue from jenkins if possible, and later implement skip behavior.
>> Otherwise, considering we have >10 people having ability to merge patches,
>> many people may miss having a look on clang-format issues.
>>
>
> I agree that it could be hard to enforce some rules, but what I'm seeing
> lately is that the clang-format version from Fedora 33 doesn't format the
> code the same way as a previous version with the same configuration in some
> cases (this also seems to happen with much older versions). This causes
> failures in the clang check that need manual modifications to update the
> patches.
>

Ok, let's get moving with actual work than syntaxes. Ok with skipping!


> Xavi
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.gluster.org/pipermail/gluster-devel/attachments/20210211/d7670463/attachment-0001.html>