wakaama icon indicating copy to clipboard operation
wakaama copied to clipboard

Automatic code formatting

Open rettichschnidi opened this issue 3 years ago • 10 comments

Historically, the Wakaama code base did not use any kind of code formatting tools. Therefore, every file looks slightly different, making it more hard than necessary to read the code, figuring out and sticking to the predominant style.

Therefore, for C code I propose to pick clang-format (version 10 for now), using a code style close to the one LLVM, the creators of clang-format, use. Foreseeable adaptions might be:

  • IndentWidth: 4 (instead of 2)
  • ColumnLimit: 120 (instead of 80; current often oversteps the 80 characters boundary)

For CMake files, https://github.com/cheshirekow/cmake_format seems to be the tool to pick.

Acceptance criteria:

  • Changes must be done in commits which are limited exclusively to cosmetic/style changes
  • Above commit id(s) can be stored in a --ignore-revs-file file (supported since Git 2.23).
  • GitHub Action in place to prevent merging of non-formatted code
  • Tools and exact versions to use defined
  • Files in coap/er-coap-13 (and examples/shared/tinydtls) should not be touched
  • Code and comments checked for being garbled, exceptions in place where sensible did not format the whole code base

Please note:

  • This is issue complements #513

Any opinions on this?

rettichschnidi avatar Apr 02 '21 18:04 rettichschnidi

I guess we definitely need something like this. :+1:

About tools to use and format rules, I let the team decide what best fits.

sbernard31 avatar Apr 06 '21 10:04 sbernard31

I am really in favor of picking a standard like clang-format as suggested, as I do not see any real advantage in relation to the effort to maintain our own standard (or deviation from an existing standard).

tuve avatar Apr 12 '21 09:04 tuve

I'm in favor of adopting a code standard. No strong opinions on the specifics, what is suggested above works for me. Before any decision is made we should get input from @sbertin-telular also.

qleisan avatar Apr 13 '21 09:04 qleisan

I'm also in favor of a code standard with no strong opinions on the specifics. If it can easily be automated and enforced even better.

sbertin-telular avatar Apr 19 '21 15:04 sbertin-telular

I tried different clang-format style definitions to approximate the style being used most often. However, always ended up with man thousands of line changes. And while using --ignore-revs-file would work for Git itself, many 3rd-party-tools (like SonarQube) do not support it, blaming me for all the issues... :/

Therefore, I implemented a partial solution for this issue: #595

It enforces the code style on new code, which is far less intrusive than formatting all existing code. Would like to get some feedback on it.

rettichschnidi avatar Apr 23 '21 22:04 rettichschnidi

With #595 merged, only the CMake formatting capabilities are missing.

rettichschnidi avatar Apr 27 '21 12:04 rettichschnidi

This is in reference to https://github.com/eclipse/wakaama/pull/602#issuecomment-836774135. It seems off topic there.

A pure cosmetic patch which reformats just the portion of code changed creates a burden on the developer. They need to make their changes to find out what needs reformatting, reformat, commit, and re-apply their changes. Repeat if more changes are requested in the pull request. Meanwhile unchanged code doesn't get the formatting updated. This could lead to a real mess. I'd rather see the code reformatted entirely (maybe 1 file per pull request to be more manageable) and move forward from there.

sbertin-telular avatar May 10 '21 14:05 sbertin-telular

Took a stab at the remaining linting/formatting of CMake code in ##653.

Since most of the CMake code got changed for #653, I went on to reformat all files, and therefore it will be possible to just run cmake-format on all CMake files like this:

git ls-files '*CMakeLists.txt' '*.cmake' | xargs cmake-lint

This is in reference to #602 (comment). It seems off topic there.

A pure cosmetic patch which reformats just the portion of code changed creates a burden on the developer. They need to make their changes to find out what needs reformatting, reformat, commit, and re-apply their changes. Repeat if more changes are requested in the pull request. Meanwhile unchanged code doesn't get the formatting updated. This could lead to a real mess. I'd rather see the code reformatted entirely (maybe 1 file per pull request to be more manageable) and move forward from there.

@sbertin-telular This will not happen for the CMake files.

rettichschnidi avatar Jan 30 '22 15:01 rettichschnidi

With #655 merged, this should be done for good. If the partial formatting of C code turns out to be too much of an issue, please reopen or create a new issue.

rettichschnidi avatar Feb 08 '22 21:02 rettichschnidi

The current method (require new code do be properly formatted) actually requires (cosmetic) changes quite far apart from the actual code change. This either results in (too) noisy commits (a) or, when striving for separation of functional and cosmetic changes, requires a follow-up commit (b):

  • https://github.com/eclipse/wakaama/pull/685/commits/6b8d0fa0952bcd80f0801d6ab99464d4857853c1
  • https://github.com/eclipse/wakaama/pull/691/commits/e89761bb376ef2bede966a6b027464b1571cbcbb

(a) makes it hard to reason about and reviews contributions and (b) is not something we can seriously ask from any contributors IMHO.

Therefore, I can also to the conclusion that reformatting the entire code base is the way to go. I will create a PR shortly.

rettichschnidi avatar Apr 13 '23 12:04 rettichschnidi