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

Script and Workflows for automatic code formating #175

Merged
merged 7 commits into from
Dec 20, 2021

Conversation

tquatmann
Copy link
Member

@tquatmann tquatmann commented Dec 16, 2021

This PR adds automatic code formatting facilities (using clang-format).

Code Formatting with Continuous Integration

Two GitHub workflows are added:

  • check-code-format fails if the code is currently not formatted correctly. This action is triggered for every push and for every pull request.
  • apply-code-format actually applies the code formatting and commits creates a pull request (like this) containing all necessary changes. It also adds commit hashes to a new .git-blame-ignore-revs file so that git blame remains (more or less) usable. The apply-code-format has to be triggered manually.

All source code files in ./src. are considered, where the file .clang-format-ignore lists directory path patterns that are excluded. Currently, everything related to DFTs, POMDPs, and parametric models is excluded.

Code Formatting Locally

This PR also adds a script for applying code formatting locally. The script can be invoked via a new make target. In the build directory, we can now run

make format

to apply the formatting to all files (without committing anything).

Alternatively, most IDEs (e.g. CLion) can be configured in a way that code formatting is applied automatically when saving.

Our .clang-format configuration currently requires clang-format to be available in at least version 12.

Once this PR is merged, the plan is to (manually) trigger the apply-code-format action so that formatting will be applied to the existing codebase. This is what will happen.

Further notes:

  • This is the same commit as above without white-space changes (obtained by appending ?w=1 to the URL) so that we can see what the formatter is actually doing.
  • Apparently, clang-format needed two runs to catch everything. This is the result of the second run. I changed the workflow so that it now runs clang-format twice before committing.
  • I also made some changes to the .clang-format configuration that disables function definitions and if-statements in a single line.

@tquatmann
Copy link
Member Author

The check-code-format action is expected to fail, because the code formatting hasn't been applied, yet.

@sjunges
Copy link
Contributor

sjunges commented Dec 16, 2021

Hi Tim,

Thanks for this. The formatting rules look good to me.
I was thinking whether it makes sense to apply it in two steps, however.
Noticably, a lot of the history gets completely broken (in order to see the history at a line) and my feeling is that that could be due to the fact that we apply some rules like indention changes and some more local changes at the same time. I think it would be very cool if we would e.g. first only change the indention based on the namespaces and then the other formatting rules. I don't want to create a lot of work here, so I leave it up to you how to handle it. My main motivation would indeed be that keeping the line-based history a bit more intact will be helpful downstream.

@tquatmann
Copy link
Member Author

Yeah, I totally see the problem with the history.

However, I somehow don't get why it helps to split these into two commits? Can you elaborate?

I just learned there is an option for git blame to ignore whitespace changes. Also, we can collect those formatting commits in a .git-blame-ignore-revs. Then, it's possible to invoke git blame in a way that those commits are ignored entirely. I think that should help.

@sjunges
Copy link
Contributor

sjunges commented Dec 16, 2021

So if I look at the link you provided with the changes made, then git seems to have issues to map the lines adequately. Instead, it is making huge blocks of removed/added stuff. I dont think the .git_blame_ignore_revs will work. My guess/experience with git is that if you e.g. only do limited trailing whitespace changes (but not whitespace / line breaks inside a line) it handles it better. My hope would be that then the blocks that are deleted/added are smaller and thus more easy to understand the original source later.

clangFormatVersion: 12
style: file
inplace: True
- uses: EndBug/add-and-commit@v4
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe it is a bit safer to open a PR instead of directly pushing a new commit? There are Github actions for automatically opening PRs.

@tquatmann
Copy link
Member Author

Thanks for your input @sjunges and @volkm ! I addressed your concerns in the (edited) description of this PR so we can have it as a reference for later.

@tquatmann tquatmann merged commit 55c76db into moves-rwth:master Dec 20, 2021
@tquatmann tquatmann deleted the auto-format branch April 3, 2022 08:13
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.

3 participants