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

[Xavi Hernandez]

On Thu, Feb 11, 2021 at 5:50 PM Yaniv Kaul <ykaul at redhat.com> wrote:

>
>
> On Thu, Feb 11, 2021 at 5:54 PM Amar Tumballi <amar at kadalu.io> wrote:
>
>>
>>
>> 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!
>>
>
Before skipping I think it would be interesting to see if your idea of
posting the result of the clang-format test as a review comment with the
suggested changes is possible. It would be very easy then to check if they
make sense or not before merging.

@Deepshikha Khandelwal <dkhandel at redhat.com> do you know if it's possible ?


> If we could run a specific version within a container...
>

Even if we run it inside a container, sooner or later that container will
need to be upgraded to newer versions of software and libraries. When
clang-format is upgraded, patches will start modifying things that the
author didn't really touch, adding unnecessary and undocumented changes to
the gluster history. Additionally, it's not possible to automatically
format everything in the best possible way because in some cases one format
will be better than another (for example for readability), but in some
other cases the same code structure will be better represented in another
way.

We have the possibility of disabling clang-format in specific parts of the
code via a special comment, but I'm not sure if it's the right solution
either.

Regards,

Xavi

Y.
>
>>
>>
>>> Xavi
>>>
>> -------
>>
>> Community Meeting Calendar:
>> Schedule -
>> Every 2nd and 4th Tuesday at 14:30 IST / 09:00 UTC
>> Bridge: https://meet.google.com/cpu-eiue-hvk
>>
>> Gluster-devel mailing list
>> Gluster-devel at gluster.org
>> https://lists.gluster.org/mailman/listinfo/gluster-devel
>>
>>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.gluster.org/pipermail/gluster-devel/attachments/20210215/a356a5e9/attachment.html>