On Mon, Feb 15, 2021 at 12:29 PM Xavi Hernandez <xhernandez@xxxxxxxxxx> wrote:
On Thu, Feb 11, 2021 at 5:50 PM Yaniv Kaul <ykaul@xxxxxxxxxx> wrote:On Thu, Feb 11, 2021 at 5:54 PM Amar Tumballi <amar@xxxxxxxxx> wrote:On Thu, 11 Feb, 2021, 9:19 pm Xavi Hernandez, <xhernandez@xxxxxxxxxx> wrote:On Wed, Feb 10, 2021 at 1:33 PM Amar Tumballi <amar@xxxxxxxxx> wrote:On Wed, Feb 10, 2021 at 3:29 PM Xavi Hernandez <xhernandez@xxxxxxxxxx> 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 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,XaviY.-------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@xxxxxxxxxxx
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@xxxxxxxxxxx https://lists.gluster.org/mailman/listinfo/gluster-devel
