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

Add a formatting style and apply it to the entire repo #75

Merged
merged 24 commits into from
Nov 15, 2024

Conversation

TimSiebert1
Copy link
Collaborator

This PR adds

  • a .clang-format file
  • a clang-format worfklow for checking the formatting style of a PR or push

Based on the .clang-format style, the entire repo is formatted. This will potentially cause many errors that will be fixed here.

@CLAassistant
Copy link

CLA assistant check
Thank you for your submission! We really appreciate it. Like many open source projects, we ask that you all sign our Contributor License Agreement before we can accept your contribution.
1 out of 2 committers have signed the CLA.

✅ TimSiebert1
❌ Clang Robot


Clang Robot seems not to be a GitHub user. You need a GitHub account to be able to sign the CLA. If you have already a GitHub account, please add the email address used for this commit to your account.
You have signed the CLA already but the status is still pending? Let us recheck it.

@TimSiebert1
Copy link
Collaborator Author

@awalther1 is the license check required? I used a bot to format everything. The bot can not sign the license :D

@TimSiebert1 TimSiebert1 marked this pull request as draft November 12, 2024 15:09
@awalther1
Copy link
Contributor

I think that in this case it is OK to proceed without the signed licence ;-)

@cgraeser
Copy link
Contributor

cgraeser commented Nov 13, 2024

When checking which style is used to update my PR I noticed that you're running clang-format on ubuntu-latest for the workflow. AFAIK different versions of clang-format do not necessarily produce consistent results for the same settings. We stumbled upon this elsewhere (but with uncrustify). To avoid such issues it may be helpful to fix the version for the underlying distribution.

@TimSiebert1
Copy link
Collaborator Author

TimSiebert1 commented Nov 13, 2024

When checking which style is used to update my PR I noticed that you're running clang-format on ubuntu-latest for the workflow. AFAIK different versions of clang-format do not necessarily produce consistent results for the same settings. We stumbled upon this elsewhere (but with uncrustify). To avoid such issues it may be helpful to fix the version for the underlying distribution.

Thanks for the hint!
The current workflow was simply to apply the format and wait to see what happens. The current workflow tries to format and will not give you any warning or error if you didnt follow the style. Once I have fixed the format, I will change the workflow such that it errors when the style is not applied and the os will be fixed too.

@TimSiebert1
Copy link
Collaborator Author

@cgraeser I decided to go with the latest ubuntu and clang-format versions in the action, since I would expect a higher chance that the most recent version of clang-format handles the format equally for different OS. On the other hand on systems like MacOS only a limited amount of older versions are available on brew. If we have constant trouble with the formatter because the latest clang-format version on various machines creates a different format or the github action errors because it utilizes a different version, I would create a git hook based on nixos. This would allow us to ultimately fix the os and version for the github runner and between every contributor of the package.

I also dont want to add a bot that automatically commits the formatted files when there are formatting issues for a PR, because of the acceptance with the license.

@cgraeser
Copy link
Contributor

My main worry was not about the version being too new. I just wanted to hint at the fact that the precise definition of ubuntu-latest and the incorporated clang-format version may change in a few months. Thus e.g. using ubuntu-24.10 would be more robust and precise.

@TimSiebert1
Copy link
Collaborator Author

Hi @cgraeser

what I meant above was that the robustness of the github action alone is not that helpful since every contributor would need the version that is given by the fixed ubuntu version. I would expect that this is not possible for different OS or at least it is not that easy. To gain a bit of robustness, I would assume that the newest version of clang-format for the latest OS is easier to obtain. However, if we have constant issues with it, I will change it.

@TimSiebert1
Copy link
Collaborator Author

What do you think about dumping the llvm style into the .clang-format to precisely fix the style? That should make the formatting a bit more robust.

@TimSiebert1
Copy link
Collaborator Author

EDIT: To make the workflow more robust (as proposed by @cgraeser)

  • I fixed the clang-format version to 19.1.3, which is available on pip
  • the llvm style is bumped to .clang-format

@TimSiebert1 TimSiebert1 marked this pull request as ready for review November 15, 2024 09:07
@TimSiebert1 TimSiebert1 merged commit 15e49d7 into coin-or:master Nov 15, 2024
14 of 16 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants