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

[Yaniv Kaul]

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!
>

If we could run a specific version within a container...
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/20210211/eef9869c/attachment.html>