Skip to content
New issue

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

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

Already on GitHub? Sign in 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!

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

Successfully merging this pull request may close these issues.

2 participants