-
-
Notifications
You must be signed in to change notification settings - Fork 2.6k
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
Add Reviewdog as checkstyle #6996
Merged
Merged
Changes from all commits
Commits
Show all changes
9 commits
Select commit
Hold shift + click to select a range
ca0284f
Add Reviewdog as checkstyle
Siedlerchr 4da7839
try with dir
Siedlerchr 23e78f2
try dir
Siedlerchr 489cac4
Merge remote-tracking branch 'upstream/master' into checkstyleConfig
Siedlerchr dee6594
test witout gradle action
Siedlerchr 4c06293
test checkstyle
Siedlerchr f019ed1
use copy for reviewdog
Siedlerchr c630c05
changke back to PR check
Siedlerchr 308fc68
fix checkstyle issue
Siedlerchr File filter
Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
There are no files selected for viewing
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,141 @@ | ||
<?xml version="1.0"?> | ||
<!DOCTYPE module PUBLIC | ||
"-//Puppy Crawl//DTD Check Configuration 1.3//EN" | ||
"http://www.checkstyle.org/dtds/configuration_1_3.dtd"> | ||
|
||
<module name="Checker"> | ||
<property name="charset" value="UTF-8"/> | ||
|
||
<module name="Header"> | ||
<property name="header" value=""/> | ||
</module> | ||
|
||
<module name="SuppressionFilter"> | ||
<property name="file" value="config/checkstyle/suppressions.xml"/> | ||
</module> | ||
|
||
<module name="BeforeExecutionExclusionFileFilter"> | ||
<property name="fileNamePattern" value="module\-info\.java$"/> | ||
</module> | ||
|
||
<module name="FileTabCharacter"/> | ||
<module name="NewlineAtEndOfFile"/> | ||
|
||
<!-- Checks for size violations: https://checkstyle.sourceforge.io/config_sizes.html --> | ||
|
||
<!-- LineLength not in place as PreviewerViewer and RelatedArticlesTab have line length with more than 500 charachters --> | ||
|
||
<module name="TreeWalker"> | ||
<!-- Checks for Javadoc comments: https://checkstyle.org/config_javadoc.html --> | ||
<module name="InvalidJavadocPosition"/> | ||
<module name="JavadocMethod"> | ||
<property name="allowMissingParamTags" value="true"/> | ||
<property name="allowMissingReturnTag" value="true"/> | ||
</module> | ||
|
||
<!-- Checks for imports: https://checkstyle.org/config_import.html --> | ||
|
||
<module name="UnusedImports"/> | ||
<module name="RedundantImport"/> | ||
<module name="AvoidStarImport"/> | ||
<module name="IllegalImport"/> | ||
<module name="ImportOrder"> | ||
<property name="groups" value="java,javax,javafx,org.jabref,*"/> | ||
<property name="ordered" value="true"/> | ||
<property name="separated" value="true"/> | ||
<property name="option" value="bottom"/> | ||
<property name="sortStaticImportsAlphabetically" value="true"/> | ||
</module> | ||
|
||
<!-- Checks for whitespace: https://checkstyle.org/config_whitespace.html --> | ||
|
||
<module name="EmptyForInitializerPad"/> | ||
<module name="EmptyLineSeparator"> | ||
<!-- check all except variable declarations --> | ||
<property name="tokens" | ||
value="IMPORT, STATIC_IMPORT, CLASS_DEF, INTERFACE_DEF, ENUM_DEF, STATIC_INIT, INSTANCE_INIT, METHOD_DEF, CTOR_DEF"/> | ||
<property name="allowMultipleEmptyLines" value="false"/> | ||
<property name="allowMultipleEmptyLinesInsideClassMembers" value="false"/> | ||
</module> | ||
<module name="GenericWhitespace"/> | ||
<module name="MethodParamPad"/> | ||
<module name="NoLineWrap"/> | ||
<module name="NoWhitespaceAfter"/> | ||
<module name="NoWhitespaceBefore"/> | ||
<module name="ParenPad"/> | ||
<module name="SeparatorWrap"> | ||
<property name="tokens" value="COMMA, SEMI, ELLIPSIS, ARRAY_DECLARATOR, RBRACK, METHOD_REF"/> | ||
</module> | ||
<module name="SingleSpaceSeparator"/> | ||
<module name="WhitespaceAfter"/> | ||
<module name="WhitespaceAround"> | ||
<!-- RCULRY causes issues if classes are nested within arrays, therefore not activated --> | ||
<property name="tokens" | ||
value="ASSIGN, BAND, BAND_ASSIGN, BOR, BOR_ASSIGN, BSR, BSR_ASSIGN, BXOR, BXOR_ASSIGN, COLON, DIV, | ||
DIV_ASSIGN, DO_WHILE, EQUAL, GE, GT, LAND, LCURLY, LE, LITERAL_CATCH, LITERAL_DO, LITERAL_ELSE, | ||
LITERAL_FINALLY, LITERAL_FOR, LITERAL_IF, LITERAL_RETURN, LITERAL_SWITCH, LITERAL_SYNCHRONIZED, | ||
LITERAL_TRY, LITERAL_WHILE, LOR, LT, MINUS, MINUS_ASSIGN, MOD, MOD_ASSIGN, NOT_EQUAL, | ||
PLUS, PLUS_ASSIGN, QUESTION, | ||
SL, SLIST, SL_ASSIGN, SR, SR_ASSIGN, STAR, STAR_ASSIGN, LITERAL_ASSERT, TYPE_EXTENSION_AND"/> | ||
</module> | ||
|
||
<!-- Checks for Naming Conventions: https://checkstyle.org/config_naming.html --> | ||
<module name="ConstantName"> | ||
<property name="format" value="^log(ger)?|[A-Z][A-Z0-9]*(_[A-Z0-9]+)*$"/> | ||
</module> | ||
|
||
<!-- Checks for blocks: https://checkstyle.org/config_blocks.html --> | ||
|
||
<module name="AvoidNestedBlocks"/> | ||
|
||
<module name="NeedBraces"/> | ||
|
||
<module name="EmptyBlock"> | ||
<property name="option" value="text"/> | ||
</module> | ||
|
||
<!-- Disallows empty catch blocks (not even having a comment): https://checkstyle.sourceforge.io/config_blocks.html#EmptyCatchBlock --> | ||
<module name="EmptyCatchBlock"/> | ||
|
||
<!-- | ||
following rule enforces that there are no one line statements such as | ||
|
||
public String getTabName() { return Localization.lang("XMP metadata"); } | ||
|
||
--> | ||
<module name="LeftCurly"/> | ||
|
||
<module name="RightCurly"/> | ||
|
||
<!-- coding - https://checkstyle.sourceforge.io/config_coding.html --> | ||
|
||
<module name="AvoidDoubleBraceInitialization"/> | ||
|
||
<module name="CovariantEquals"/> | ||
|
||
<module name="MultipleVariableDeclarations"/> | ||
|
||
<module name="OneStatementPerLine"> | ||
<property name="treatTryResourcesAsStatement" value="true"/> | ||
</module> | ||
|
||
<module name="UnnecessarySemicolonInTryWithResources"/> | ||
|
||
<!-- Checks for common coding problems: https://checkstyle.org/config_coding.html --> | ||
|
||
<module name="DeclarationOrder"/> | ||
<module name="EmptyStatement"/> | ||
|
||
<module name="EqualsHashCode"/> | ||
|
||
<!-- force a space after // for comments --> | ||
<module name="TodoComment"> | ||
<property name="id" value="commentStartWithSpace"/> | ||
<property name="format" value="^([^\s\/*])"/> | ||
<message key="todo.match" value="Comment text should start with space."/> | ||
</module> | ||
|
||
<module name="MissingDeprecated"/> | ||
|
||
</module> | ||
</module> |
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Do we really need to duplicate the checkstyle config? Makes it hard in the future to keep them synced
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Checks are also possible aka the default, but only visible if you look at the files changed tab.
I personally think this comment variant is useful
Unfortunately we currently need this duplication because otherwise the suppression config file cannot be found due to the parameter expansion or the gradle check cannot be run.
There's also no way to inline the suppression
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It just adds so much noise, if you commit half-finished PRs that still need cleanup etc. You then get github notifications because a bot commented. Why not educate people to look at all checks?
Currently, the checkstyle issues do not show up in the changed files tab.
So with the current config, reviewdog will report about things that we actually decided to suppress?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I changed back to github checks feature.
Again, for the copy. Reviewdog needs a copy of the checkstyle.xml because it contains the relative path to the supression.xml file. Otherwise it cannot parse the ${configloc} variable.
Gradle and the IDEs need that variable to automatically set the correct path
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks!
Can you then please open an issue at reviewdog so that this gets fixed in the future. Thanks!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's more a checkstlye issue (that won't be fixed). However, I opened an issue at the action repo.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The issue is nikitasavinov/checkstyle-action#16. It should have been solved with nikitasavinov/checkstyle-action#19. Should we try it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Try it...