On Mon, Feb 15, 2021 at 12:29 PM Xavi Hernandez <xhernan...@redhat.com> wrote:
> > On Thu, Feb 11, 2021 at 5:50 PM Yaniv Kaul <yk...@redhat.com> wrote: > >> >> >> On Thu, Feb 11, 2021 at 5:54 PM Amar Tumballi <a...@kadalu.io> wrote: >> >>> >>> >>> On Thu, 11 Feb, 2021, 9:19 pm Xavi Hernandez, <xhernan...@redhat.com> >>> wrote: >>> >>>> On Wed, Feb 10, 2021 at 1:33 PM Amar Tumballi <a...@kadalu.io> wrote: >>>> >>>>> >>>>> >>>>> On Wed, Feb 10, 2021 at 3:29 PM Xavi Hernandez <xhernan...@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 <dkhan...@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@gluster.org >>> https://lists.gluster.org/mailman/listinfo/gluster-devel >>> >>>
------- 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@gluster.org https://lists.gluster.org/mailman/listinfo/gluster-devel
