search

jamulussoftware / jamulus

Jamulus enables musicians to perform real-time jam sessions over the internet.
https://jamulus.io
Other
997 stars 222 forks source link

Automatic code linting for pull requests #901

Closed ann0see closed 3 years ago

ann0see commented 3 years ago

I talked to corrados via mail and thought of automating some more steps in GitHub.

Reviewing a PR can be automated to some extent via a linter which enforces code style.

Unfortunately, I couldn't find a style guide (+ linter) which would fit to the Jamulus style, especially considering the two spaces between arguments in e.g. function calls/if statements:

if ( a ) seems to be uncommon.

Does anybody know a linter which supports custom styles and a GitHub action?

pljones commented 3 years ago

I know that many editors and IDEs allow custom style guides (and auto-complete automation). I'd expect linters to support similar customisation. I've not looked though - but I'd be very surprised if they didn't.

passing commented 3 years ago

if ( a ) seems to be uncommon.

I am having a look at clang-format which I found has options for this https://clang.llvm.org/docs/ClangFormatStyleOptions.html

SBPO_NonEmptyParentheses (in configuration: NonEmptyParentheses) Put a space before opening parentheses only if the parentheses are not empty i.e. ‘()’

Still trying to find some configuration that fits best to the existing code ...

I just noticed that right now some code is aligned to 80 characters and some is not. So either all code would need to get aligned to that or to a bigger limit - to be defined...

passing commented 3 years ago

Got quite far with the configuration for clang-format which I put on this fork now: https://github.com/passing/jamulus/blob/master/.clang-format It is based on the 'llvm' style which you can get with

clang-format --style=llvm -dump-config

documentation for that is available here: https://releases.llvm.org/10.0.0/tools/clang/docs/ClangFormatStyleOptions.html

I have also setup this github action: https://github.com/marketplace/actions/clang-format-lint The result from that can be viewed here: https://github.com/passing/jamulus/commit/d93f101629532ff31d7c7f34632fb20bd7583df3 most of the changes are due to the column limit of 80 - if you increase that the number of changes is still very high as many lines get merged then instead.

if you want to try different settings, you can just put the configuration in your repo folder locally and run something like

clang-format -i server.cpp ; git diff ; git checkout server.cpp
ann0see commented 3 years ago

Sounds great! So it also automatically does the changes?

passing commented 3 years ago

here's an updated configuration which now only contains the options that differ from the LLVM base style: https://github.com/passing/jamulus/blob/master/.clang-format by setting ReflowComments: false existing comments will not get touched this commit shows all the changes the linter would do now: https://github.com/passing/jamulus/commit/62ba01d6828701fea011be2703e2e44352be30dc

So it also automatically does the changes?

Yes, it just adds another commit: https://github.com/passing/jamulus/commits/master

I am not sure if it is possible to use this on pull requests coming from a fork. I added pull-requests to the triggers now: https://github.com/passing/jamulus/blob/master/.github/workflows/clang-format-linter.yml#L3 ...so would be good if someone can try to create a PR to my repo

passing commented 3 years ago

@ann0see looks like it works on a PR: https://github.com/passing/jamulus/pull/2

Is it possible that the robot doesn't automatically commit but suggest changes? I'm afraid that it might break something.

It should usually never break the code as it basically just wraps things around. Of course there could be a bug in the linter itself, but that I assume to be very unlikely. By defining the code style like this means there is just one way for every line of code that is right. If we just suggest and don't change the code, the list of suggestions (that have not been applied) will get longer and longer over time. So I'd rather suggest to rely on the automation and fix that in case something goes wrong - in the end you can always revert a commit somehow.

hoffie commented 3 years ago

By defining the code style like this means there is just one way for every line of code that is right.

... and I think this is very important to avoid edit wars, both manual and automatical.

As said elsewhere, I'm really happy that there is a consistent coding style so I'm absolutely in favor of keeping one. On the other hand, it was really hard for me (in one of my PRs) to spot all places where I've gotten the spacing wrong. Seems like other people are having similar issues (https://github.com/jamulussoftware/jamulus/pull/945#discussion_r571508818).

I would welcome both changes to make local linting just work (some make lint target or something) and something which enforces, checks and/or corrects this centrally on Github.

passing commented 3 years ago

So now there's 2 things to be decided:

1st: is the code style produced by clang-format with the proposed configuration (https://github.com/jamulussoftware/jamulus/issues/901#issuecomment-774488549) ok in general?

2nd: how should the integration look like. I see lots options:

  1. Add github action so style is enforced automatically.
  2. Add github action so style is checked. Invalid style blocks the merge.
  3. Add github action so style is checked. Invalid style does not block the merge.
  4. make lint target (not sure how this can be done to work on all platforms)
  5. Just add the .clang-format file to the project so IDEs can use it + lint manually once and commit the changes.
hoffie commented 3 years ago

1) I've scanned the https://github.com/passing/jamulus/commit/62ba01d6828701fea011be2703e2e44352be30dc and I like most of the changes there. It makes things more readable and/or more consistent. There are a few places where it looks like intentional formatting gets "destroyed", but I fear it might not be easy to avoid that. Some examples: https://github.com/passing/jamulus/commit/62ba01d6828701fea011be2703e2e44352be30dc#diff-32ef006d0b2b2daef9b1dc9c1e43c991a5b41822a1a5ef972592c6d1b636ca48L48 https://github.com/passing/jamulus/commit/62ba01d6828701fea011be2703e2e44352be30dc#diff-32ef006d0b2b2daef9b1dc9c1e43c991a5b41822a1a5ef972592c6d1b636ca48L123 https://github.com/passing/jamulus/commit/62ba01d6828701fea011be2703e2e44352be30dc#diff-32ef006d0b2b2daef9b1dc9c1e43c991a5b41822a1a5ef972592c6d1b636ca48L259 https://github.com/passing/jamulus/commit/62ba01d6828701fea011be2703e2e44352be30dc#diff-6cf4397b462c3ba68cf479657463c05bedfcb3e6b839f4979c22fc1fa1985a58L206 https://github.com/passing/jamulus/commit/62ba01d6828701fea011be2703e2e44352be30dc#diff-801be6be7a2afd8dd1cd11b007a6020a41083a439e2a7e02247115087dadb71eL35

2) Helping contributors to commit properly formatted code in the first place makes reading history/commits/diffs way easier, so I'd vote for that (4). This should still be enforced somehow (1 or 2). Option 5 would be better than nothing, but still really weak.

passing commented 3 years ago

passing@62ba01d#diff-32ef006d0b2b2daef9b1dc9c1e43c991a5b41822a1a5ef972592c6d1b636ca48L48

align on arrow operator doesn't seem to be possible

passing@62ba01d#diff-32ef006d0b2b2daef9b1dc9c1e43c991a5b41822a1a5ef972592c6d1b636ca48L123

the AlignConsecutiveAssignments gets overridden by the default ColumnLimit: 80. Changing that to ColumnLimit: 120 would make it nicer (here)

passing@62ba01d#diff-32ef006d0b2b2daef9b1dc9c1e43c991a5b41822a1a5ef972592c6d1b636ca48L259

ColumnLimit: 120 would keep it like it is

passing@62ba01d#diff-6cf4397b462c3ba68cf479657463c05bedfcb3e6b839f4979c22fc1fa1985a58L206

sorting of include blocks could be adjusted, e.g. there's IBS_Preserve = Sort each #include block separately.

passing@62ba01d#diff-801be6be7a2afd8dd1cd11b007a6020a41083a439e2a7e02247115087dadb71eL35

that kind of alignment also doesn't seem to be possible

hoffie commented 3 years ago

Thanks for your further analysis!

The diff is rather big, but I would still say that someone should scan through all of it. I'm volunteering to do that and would highlight anything controversial so that anyone can chime in. Maybe there are still some optimizations possible. However, this will be some effort and it would also do that if there's consensus to move this forward.

I really like what @passing has done so far and think it is important to tackle this issue rather sooner than later as it will hopefully make our work (and the work by contributors) easier.

Regarding the questions above:

@jamulussoftware/maindevelopers What do you think?

If there's consensus I'd ask @passing to open a PR where we could go through the initial diff.

pljones commented 3 years ago

Personally, I think non-blocking is better: the simple reason is that sometimes it's more urgent to get a fix than it is to worry about someone having got spacing wrong. That can be fixed later, so long as the linter is still highlighting it.

That does mean aggressively fixing lint issues when highlighted to keep the number very low and increase visibility.

It also means issues where the linter cannot be trained to accept "correct style" can be commented as "// lint accepted" or something (ideally a directive to the linter to shut up about specific issues on specific lines via a config would be nice).

hoffie commented 3 years ago

Personally, I think non-blocking is better: the simple reason is that sometimes it's more urgent to get a fix than it is to worry about someone having got spacing wrong. That can be fixed later, so long as the linter is still highlighting it.

Good point. I assume Github owners might still have the power to override, even in blocking mode. If this is the case, I'd still prefer this. If this isn't possible, then we should probably go for non-blocking. @passing Is this something you could test on your fork?

ideally a directive to the linter to shut up about specific issues on specific lines via a config would be nice

I agree.

passing commented 3 years ago

thanks for the feedback!

Then I'll take the following tasks with me

I do not have much experience with make and how to deal with tool dependencies there, it would be good if somebody could help out with

passing commented 3 years ago

I have created the PR for checking all changes implied by the clang-format style: https://github.com/jamulussoftware/jamulus/pull/1127

regarding

check how to add style exceptions in the code

that could be done with Disabling Formatting on a Piece of Code:

int formatted_code;
// clang-format off
    void    unformatted_code  ;
// clang-format on
void formatted_code_again;
pljones commented 3 years ago

... though I'd pickilly prefer void niceSpacing() { shouldHaveSpaces( /* clang-format off */42+11/* clang-format on */ ); } style for tight control. But having the control is great!

I'll take a look at the change tomorrow.

gilgongo commented 3 years ago

Is this done now? Can we close?

hoffie commented 3 years ago

Is this done now? Can we close?

No, this is a huge undertaking and the PR (#1127) is still in flight. I've now linked it to this issue. We will also have to check if the issue is done or if there are still follow-ups to do (possibly new issues, but we'll need tracking for that). The PR handles the format definition and the initial linting. I think any automatisms are not in the scope of that PR yet and will have to be done afterwards.

hoffie commented 3 years ago

With this issue having been auto-closed despite several tasks still to be done, I think it's best to keep it closed and start with a fresh outline what to do. I've done that here: #1702