Skip to content
New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Apply checkstyle plugin #2004

Draft
wants to merge 3 commits into
base: master
Choose a base branch
from
Draft

Apply checkstyle plugin #2004

wants to merge 3 commits into from

Conversation

cmnrd
Copy link
Collaborator

@cmnrd cmnrd commented Sep 13, 2023

This enables the checkstyle plugin for gradle using the google style template.

As it turns out, the checkstyle tool isn't quite the tool that I was after when I wrote #1806. Checkstyle is purely a style checker and it is quite pedantic 😅. For instance, these are the warnings produced for TimeValue.java:

[ant:checkstyle] [WARN] /home/cmenard/projects/lf/lingua-franca/core/src/main/java/org/lflang/TimeValue.java:3: Empty line should be followed by <p> tag on the next line. [JavadocParagraph]
[ant:checkstyle] [WARN] /home/cmenard/projects/lf/lingua-franca/core/src/main/java/org/lflang/TimeValue.java:6: Empty line should be followed by <p> tag on the next line. [JavadocParagraph]
[ant:checkstyle] [WARN] /home/cmenard/projects/lf/lingua-franca/core/src/main/java/org/lflang/TimeValue.java:9: Empty line should be followed by <p> tag on the next line. [JavadocParagraph]
[ant:checkstyle] [WARN] /home/cmenard/projects/lf/lingua-franca/core/src/main/java/org/lflang/TimeValue.java:13: Empty line should be followed by <p> tag on the next line. [JavadocParagraph]
[ant:checkstyle] [WARN] /home/cmenard/projects/lf/lingua-franca/core/src/main/java/org/lflang/TimeValue.java:77:3: Missing a Javadoc comment. [MissingJavadocMethod]
[ant:checkstyle] [WARN] /home/cmenard/projects/lf/lingua-franca/core/src/main/java/org/lflang/TimeValue.java:91:5: switch without "default" clause. [MissingSwitchDefault]
[ant:checkstyle] [WARN] /home/cmenard/projects/lf/lingua-franca/core/src/main/java/org/lflang/TimeValue.java:142:5: 'if' construct must use '{}'s. [NeedBraces]
[ant:checkstyle] [WARN] /home/cmenard/projects/lf/lingua-franca/core/src/main/java/org/lflang/TimeValue.java:206:13: Abbreviation in name 'isThisUnitSmallerThanBUnit' must contain no more than '1' consecutive capital letters. [AbbreviationAsWordInName]

I think this is really nice to enforce a more consistent style, but I do think that we have more pressing problems at the moment than fixing all those style warnings. Therefore, I think we shouldn't merge this PR for now. I put the PR here in hope that it will be useful at a later point.

I will open another PR that uses the spotbugs tool, which is a static code analysis tool and produces much more useful warnings. (#2005)

@lhstrh
Copy link
Member

lhstrh commented Sep 13, 2023

I think this is really nice to enforce a more consistent style, but I do think that we have more pressing problems at the moment than fixing all those style warnings. Therefore, I think we shouldn't merge this PR for now. I put the PR here in hope that it will be useful at a later point.

Agreed!

# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants