search

SERG-Delft / andy

Andy assesses student's test code. It's used in CSE1110, TU Delft.
MIT License
78 stars 23 forks source link

Adding a Checkstyle plugin #235

Closed RutaGiedryte closed 1 year ago

RutaGiedryte commented 1 year ago

I added a Checkstyle plugin to the project and a simple custom Checkstyle configuration.

This is one of the steps to solve #231 . I added a dependency to pom.xml of the parent project, both for build phase and for generating reports (if mvn site is run). Also, mvn checkstyle:checkstyle uses the default sun_checks.xml, but mvn checkstyle:check uses the custom one. If any Checkstyle rule is broken, the build will not be successful.

The custom configuration, which now contains the two checks CodeSherrif performs. Since I did not find an easy way to ignore specific method names, I just raised the bar for the allowed method length from 25 to 69. For any individual method any check can be suppressed using @SuppressWarnings (I added one example to Result builder.java).

Please let me know if there is anything to be changed, and I will gladly do that!

mauricioaniche commented 1 year ago

Great, Ruta!

Soon I’ll take a look at the rules in checkstyle and post here the ones I think we should enable. Your input is more than welcome as well!

We should get full rid of codesheriff!

Finally, does it make sense to have a step in the CI pipeline that only runs the checkstyle? This way, if there’s a checkstyle violation, it’d be very clear to see.

On Fri, 7 Jul 2023 at 20:42, Rūta Giedrytė @.***> wrote:

I added a Checkstyle plugin to the project and a simple custom Checkstyle configuration.

This is one of the steps to solve #231 https://github.com/SERG-Delft/andy/issues/231 . I added a dependency to pom.xml of the parent project, both for build phase and for generating reports (if mvn site is run). Also, mvn checkstyle:checkstyle uses the default sun_checks.xml, but mvn checkstyle:check uses the custom one. If any Checkstyle rule is broken, the build will not be successful.

The custom configuration, which now contains the two checks CodeSherrif performs. Since I did not find an easy way to ignore specific method names, I just raised the bar for the allowed method length from 25 to 69. For any individual method any check can be suppressed using @SuppressWarnings https://github.com/SuppressWarnings (I added one example to Result builder.java).

Please let me know if there is anything to be changed, and I will gladly do that!

You can view, comment on, or merge this pull request online at:

https://github.com/SERG-Delft/andy/pull/235 Commit Summary

File Changes

(3 files https://github.com/SERG-Delft/andy/pull/235/files)

Patch Links:

— Reply to this email directly, view it on GitHub https://github.com/SERG-Delft/andy/pull/235, or unsubscribe https://github.com/notifications/unsubscribe-auth/AAAYTTEQXSOG5OXJQF63R3DXPBKBPANCNFSM6AAAAAA2CE7UCI . You are receiving this because you are subscribed to this thread.Message ID: @.***>

--

Maurício Aniche Author of Effective Software Testing: A Developer's Guide https://www.effective-software-testing.com https://www.mauricioaniche.com

mauricioaniche commented 1 year ago

Let's start with these ones!

DeclarationOrder EmptyStatement EqualsHashCode MissingSwitchDefault NestedIfDepth UnusedLocalVariable Unnecessary … all of them EmptyBlock EmptyCatchBlock StringLiteralEquality

RutaGiedryte commented 1 year ago

I added all of the code checks into Checkstyle, and either added missing hashcodes, refactored the order of things or similar details. The UnnecessaryParentheses check seemed to be counterproductive sometimes as parentheses really help with ambiguity in some places, so I just suppressed it in those cases.

I also added Checkstyle into the workflow (I hope that is how it works :DD) and removed CodeSheriff.

Should I also suppress warnings for method length and set it to 25 or leave it at a bigger number, as is now? I will also look into Errorprone, probably tomorrow (Tuesday).

mauricioaniche commented 1 year ago

If you think the parenthesis rule is unnecessary, we then remove it!

For the line size, I think we can add existing methods as exception and reduce the number. If they aren't too many. How many methods offend the 25 mark?

RutaGiedryte commented 1 year ago

If you think the parenthesis rule is unnecessary, we then remove it!

For the line size, I think we can add existing methods as exception and reduce the number. If they aren't too many. How many methods offend the 25 mark?

Okay, I will remove the parentheses check :)) There are around 24 methods to violate the 25 line mark, most of them go up to around 35. Raising the bar to 35 gives 8 violations, raising to 40 leaves 4. The longest method seems to be 53 lines long.

mauricioaniche commented 1 year ago

What if we put the rule at 50, and you refactor the one or two methods that are above the threshold ?

RutaGiedryte commented 1 year ago

What if we put the rule at 50, and you refactor the one or two methods that are above the threshold ?

For now, my refactoring was removing some whitespace and making line lengths more even. Now all methods have 50 lines or less. Since the long methods create a lot of internal variables, calling another function may be even less clear than leaving it as is. I will think of a better solution and will refactor again if I come up with something clean and helpful.

mauricioaniche commented 1 year ago

Removing white lines is good enough!

Do you need to push this change ? Let me know and I think we are ready to merge!

RutaGiedryte commented 1 year ago

Removing white lines is good enough!

Do you need to push this change ? Let me know and I think we are ready to merge!

I think I have pushed everything, I believe it's ready to merge!

mauricioaniche commented 1 year ago

Thanks for the contribution!!