Check code style with Checkstyle

[wb9688]

What is it?

• Bug fix
• Feature
• Meta

Long description of the changes in your PR

This PR will let Travis check the code style of all PRs. Do you think this should be run before every build as well?

I'm also going to fix all the current code style issues, except in GigaCrap, so I excluded that already in build.gradle.

The Checkstyle configuration is based on this, but I disabled some checks.

I'm going to implement this for NewPipeExtractor as well.

Agreement

• I carefully read the contribution guidelines and agree to them.

[XiangRongLin]

How can i import this into Android Studio, so that the autoformatter will follow those rules?

[B0pol]

And another question, how to check if the code style is ok, without pushing to github and wait for Travis?

Edit: you replied to me from IRC, but I guess other people want to know (or they will want to know when this is merged).

You have to run runCheckstyle task from app/build.gradle.
To do that, either you go in android studio, in build.gradle (Module: app), and click on the run button for the task runCheckStyle, or you go in the root of NewPipe, and do ./gradlew runCheckstyle

This should be asked and required to run, and added in PR template @wb9688 @Poolitzer

[Poolitzer]

Do you think this should be run before every build as well?

Not needed if you fix everything before you merge this.

and added in PR template

Happily.

[connectety]

Am I correct in the assumption, that this is still a draft, because the build fails, because the style isn't all correct?
If not, what needs to be done before merging?

[B0pol]

You are right, and @wb9688 is fixing them. There was about 8000 issues, now about 3000.

[wb9688]

@B0pol and @Poolitzer: It doesn't need to be added to the PR template. It'll run on every build, and make the build fail when there are code style issues.

@connectety: That's correct and I'm fixing them.

[Poolitzer]

@B0pol and @Poolitzer: It doesn't need to be added to the PR template. It'll run on every build, and make the build fail when there are code style issues.

The point is telling developers to run it locally so they dont push it, geta a travis fail, figure out what the reason is, and then fix it in an unnecessary commit

[wb9688]

@Poolitzer: No, as I said, it'll run on every single build. So if you press the run or build button in Android Studio (or do e.g. ./gradlew assembleDebug), it'll fail when there are code style issues.

[XiangRongLin]

@wb9688

No, as I said, it'll run on every single build. So if you press the run or build button in Android Studio (or do e.g. ./gradlew assembleDebug), it'll fail when there are code style issues.

I would find the build failing very annoying while developing and adding some temporary (log) statements, which may not adhere to the checkstyle.

[Stypox]

Had a brief look at the first ~100 files. Thank you for this tedious work!

[wb9688]

@Stypox: Those things are probably caused by me using the Android Studio's reformatter initially on all files, and/or Git fucking up rebase.

[wb9688]

@TobiGr: I fixed everything that was pointed out by @Stypox (also in the other files he didn't check), so you could merge it now.

[wb9688]

@B0pol: Yes, it's intended that there are some warnings, but those are not errors. We should maybe disable VisibilityModifier though, or fix those warnings in another PR.

[B0pol]

I removed my message because I had no more this after rerun, but it was just a bug
Current state:

Checkstyle files with violations: 55
Checkstyle violations by severity: [warning:161]

@B0pol: Yes, it's intended that there are some warnings, but those are not errors. We should maybe disable VisibilityModifier though, or fix those warnings in another PR.

Why another pr? It's harder to find the error between 161 warnings, if you remove these warnings, only one line will show up.

also, why is newpipe.streams excluded?

[wb9688]

Why another pr? It's harder to find the error if there are 161 warnings, if you remove these warnings, gradle will show up only the line with the error.

Because it's a lot of work and not really needed. We could just disable VisibilityModifier (and maybe MissingSwitchDefault as well) in this PR though.

also, why is newpipe.streams excluded?

I didn't want to do that, since I had VisibilityModifier as error initially, and there it would be a lot of even more pointless work. However now that it's a warning or even disabled, I could do that. I also didn't know what formatting would be appropriate for certain arrays in there, or whether it was safe to just make some arguments final.

[TobiGr]

Thank your for the effort!
Tested this and does not seem to break anything. I'll leave two things open for discussion.

[Stypox]

Aaagh, loading all diffs made Firefox behave strangely ;-)
Anyway, I should have briefly looked at all files now, I found a few more little issues, and when those are solved this can be merged. I also tested a little bit on my phone (Android 7.0) by doing random things, though I don't know if it matters much. @TobiGr how do you go about testing? Do you have a list of things to do in order to make sure everything is working fine?
Thank you again!

[TobiGr]

Happy rebasing to everyone who has a PR open ;)

[XiangRongLin]

@TobiGr
I can't build the app because i get Errors in Mp4FromDashWriter

[wb9688]

@XiangRongLin: Did you change anything in that file? If so, update the line numbers in checkstyle-suppressions.xml.

[TobiGr]

@XiangRongLin That's strange because the build for the merge commit 2403184 succeded

[XiangRongLin]

@TobiGr
It say there are lines exceeding 100 chars like https://github.com/TeamNewPipe/NewPipe/blob/dev/app/src/main/java/org/schabi/newpipe/streams/Mp4FromDashWriter.java#L713 or line 725, 739, 763 724, 738, 762

Did you change anything in that file? If so, update the line numbers in checkstyle-suppressions.xml.

I'm on the dev branch without any changes. If i increase the line limit, it builds

[wb9688]

I'm on the dev branch without any changes. If i increase the line limit, it builds

@XiangRongLin: That's not possible if it fails on the lines you mention. E.g. line 725 definitely isn't longer than 100 characters.

@kapodamy: Would you mind fixing the errors I've suppressed in checkstyle-suppressions.xml anyway? As I said above:

I also didn't know what formatting would be appropriate for certain arrays in there, or whether it was safe to just make some arguments final.

[XiangRongLin]

@wb9688
Ah wait, I fixed the error in 713 by moving it into a new line, so the rest of the errors moved one line down. It should be 724, 738, 762

[wb9688]

@XiangRongLin: Have you set something else in org.checkstyle.sun.suppressionfilter.config? Try changing this to:

        <property name="file" value="checkstyle-suppressions.xml"/>

It should just work on dev. At least it does on my PC, and Travis confirms that. You could also try cloning NewPipe again.

[XiangRongLin]

@wb9688
The only error i have is in line 713, the line is 101 character long.

The reason why the other lines also appeared is because i fixed line 713 by splitting it into two lines. This causes everything below to be moved one line down, which in turn cause the errors to be in one line lower. Now they don't match the suppression and appear as errors.

Edit: this took me way too long to figure out

[wb9688]

@XiangRongLin: I don't know what you're doing, but on my PC that line is exactly 100 characters.

Edit: Maybe your PC is acting weird because of the ° in that line? What OS and Java version are you using? I'm using Debian with OpenJDK 8.

[XiangRongLin]

I tried runCheckstyle in a fresh clone on my windows pc with JDK 8 and in my ubuntu vm with openJDK 11.
I also tried changing the line ending style on checkout from git.

I'll just set the max to 101 locally and forget that this problem existed.

Edit.: It is that °, replacing that fixes it. But it should already be fixed with checkstyle/checkstyle#7845