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

Add Reviewdog as checkstyle #6996

Merged
merged 9 commits into from
Oct 24, 2020
Merged
Show file tree
Hide file tree
Changes from 8 commits
Commits
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
7 changes: 6 additions & 1 deletion .github/workflows/tests.yml
Original file line number Diff line number Diff line change
Expand Up @@ -41,7 +41,12 @@ jobs:
with:
path: ~/.gradle/wrapper
key: ${{ runner.os }}-gradle-${{ hashFiles('gradle/wrapper/gradle-wrapper.properties') }}
- name: Run checkstyle
- name: Run check style reporter
uses: nikitasavinov/checkstyle-action@master
with:
reporter: github-pr-check
checkstyle_config: 'config/checkstyle/checkstyle_reviewdog.xml'
- name: Run checkstyle gradle
run: ./gradlew checkstyleMain checkstyleTest checkstyleJmh
- name: Run markdown-lint
uses: avto-dev/markdown-lint@v1
Expand Down
141 changes: 141 additions & 0 deletions config/checkstyle/checkstyle_reviewdog.xml
Original file line number Diff line number Diff line change
@@ -0,0 +1,141 @@
<?xml version="1.0"?>
Copy link
Member

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

Copy link
Member Author

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

Copy link
Member

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?

Copy link
Member Author

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

Copy link
Member

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!

Copy link
Member Author

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.

Copy link
Member

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?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Try it...

<!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>
4 changes: 4 additions & 0 deletions src/main/java/org/jabref/gui/maintable/MainTable.java
Original file line number Diff line number Diff line change
Expand Up @@ -67,6 +67,10 @@ public class MainTable extends TableView<BibEntryTableViewModel> {
private long lastKeyPressTime;
private String columnSearchTerm;



//test for reviewdog
Siedlerchr marked this conversation as resolved.
Show resolved Hide resolved
Siedlerchr marked this conversation as resolved.
Show resolved Hide resolved

public MainTable(MainTableDataModel model,
BasePanel panel,
BibDatabaseContext database,
Expand Down