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

[Deepshikha Khandelwal]

On Mon, Feb 15, 2021 at 12:29 PM Xavi Hernandez <xhernandez at redhat.com>
wrote:

>
> 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
> ?
>
Yes, it is doable. I will update you once I implement it.

>
>
>> 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/20210216/0bfe8743/attachment.html>