<div dir="auto"><div><br><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Thu, 11 Feb, 2021, 9:19 pm Xavi Hernandez, <<a href="mailto:xhernandez@redhat.com">xhernandez@redhat.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div dir="ltr">On Wed, Feb 10, 2021 at 1:33 PM Amar Tumballi <<a href="mailto:amar@kadalu.io" target="_blank" rel="noreferrer">amar@kadalu.io</a>> wrote:<br></div><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr"><div dir="ltr"><br></div><br><div class="gmail_quote"><div dir="ltr" class="gmail_attr">On Wed, Feb 10, 2021 at 3:29 PM Xavi Hernandez <<a href="mailto:xhernandez@redhat.com" target="_blank" rel="noreferrer">xhernandez@redhat.com</a>> wrote:<br></div><blockquote class="gmail_quote" style="margin:0px 0px 0px 0.8ex;border-left:1px solid rgb(204,204,204);padding-left:1ex"><div dir="ltr">Hi all,<div><br></div><div>I'm wondering if enforcing clang-format for all patches is a good idea...</div><div><br></div><div>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.</div><div><br></div><div>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.</div><div><br></div><div>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.</div><div><br></div><div>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.</div><div><br></div><div>What do you think ?</div><div><br></div></div></blockquote><div><br></div><div>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.</div></div></div></blockquote><div><br></div><div>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.</div><div></div></div></div></blockquote></div></div><div dir="auto"><br></div><div dir="auto">Ok, let's get moving with actual work than syntaxes. Ok with skipping!</div><div dir="auto"><br></div><div dir="auto"><div class="gmail_quote"><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div dir="ltr"><div class="gmail_quote"><div><br></div><div>Xavi</div></div></div>
</blockquote></div></div></div>