Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Clang format proposal v5 #5222

Closed
wants to merge 4 commits into from

Conversation

roligugus
Copy link
Contributor

Continuation of #5186

Make sure these boxes are signed before submitting your Pull Request -- thank you.

Link to redmine ticket:
https://redmine.openinfosecfoundation.org/issues/3736

Describe changes:

  • Updated with review comments. Lots of small formatting adjustment, e.g.
  • 100 chars line length @victorjulien
  • 4 chars indentation for variable definitions @jasonish
  • Unify "foreach" macro formatting including TAILQ_FOREACH and all others I found. @jasonish
  • Added link to documentation in .clang-format @jasonish

Main other changes are...

  • Error in case of wrong formatting as per discussion
  • Ignoring the "merge commit" for merges to OISF repo to not complain about wrong formatting on newer commits on master
  • Using minimal .clang-format file
  • Removed sample file as well
  • Removed make targets, updated devguide to refer to helper script instead

Wrong Formatting is an Error

As per #5186 (comment), formatting produces a github action error. This will provide a better visual and show the formatting-check action as failed in your pull request...

Example formatting-check action error: https://github.com/OISF/suricata/runs/914986617?check_suite_focus=true

One can chose which actions must pass in "Branch prediction Rules" in the repo settings. You could require all actions but the "formatting-check" to pass to still allow merging even if formatting is not correct:

it seems one can pick which status checks that have to pass in your "Branch Protection Rules", see https://docs.github.com/en/github/administering-a-repository/enabling-required-status-checks

So as long as you don't require the formatting-check to pass, you can merge pull requests.

Hence, I'd recommend the formatting-check to error out in case of wrong formatting problems and not pick it as a required "status check" for merging purposes. That way, you'd see the red failure indication in the pull request, yet your merging is not blocked. I.e. it's up to you as maintainers to decide whether to request a formatting change on the pull request or merge something in anyways.

@roligugus roligugus requested review from norg, victorjulien and a team as code owners July 27, 2020 15:21
@roligugus roligugus mentioned this pull request Jul 27, 2020
3 tasks
@@ -6,12 +6,191 @@ Suricata uses a fairly strict coding style. This document describes it.
Formatting
~~~~~~~~~~

clang-format
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@victorjulien
Copy link
Member

clang-format file commit says "DO NOT MERGE AS IS."

Is that still correct?

@roligugus
Copy link
Contributor Author

clang-format file commit says "DO NOT MERGE AS IS."

Is that still correct?

No, argh! I forgot to update the commit message.

Should I fix the commit and repush this, or wait for feedback and then fix in a potential v6?

@victorjulien
Copy link
Member

Any idea what I'm missing here?
Screenshot from 2020-07-27 22-10-45

@roligugus
Copy link
Contributor Author

roligugus commented Jul 27, 2020

Any idea what I'm missing here?
Screenshot from 2020-07-27 22-10-45

Yeah, that's the messy ubuntu clang versioning... Do this
git clang-format-9 --binary clang-format-9 HEAD^

Fyi, git clang-format-9 ... will call the git-clang-format-9 script which will use the clang-format binary by default, but in in ubuntu's case should use the clang-format-9 binary.

Btw, you could also configure a different clang-format version to use if not specified - just don't forget to change this if you ever switch to a different clang-format version: git config clangformat.binary "clang-format-9" or git config --global clangformat.binary "clang-format-9" which will allow you to go back to git clang-format-9 HEAD^

Or simply use the helper script ;)

@victorjulien
Copy link
Member

Merged in #5349, thanks a lot @roligugus ! This was a much heavier lift than I would have thought beforehand. Really appreciate the work you've put into this.

Now lets see how well it will fit into our workflows :)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants