Skip to content

Clean up checkstyle warnings #2765

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

Merged
merged 12 commits into from
Apr 21, 2017
Merged

Clean up checkstyle warnings #2765

merged 12 commits into from
Apr 21, 2017

Conversation

snisnisniksonah
Copy link
Contributor

Clean up checkstyle warnings
Add checkstyle suppression for gen

@tobiasdiez tobiasdiez added the status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers label Apr 19, 2017
Copy link
Member

@LinusDietz LinusDietz left a comment

Choose a reason for hiding this comment

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

Hi @snisnisniksonah, thank you for your contribution.
Improving the readability of the code is indeed something useful!

While I honor the contribution, I would kindly ask you to change the destination of this PR to JabRef's master branch instead of checkstyle. (unless, of course, you have been in contact with @koppor before)
Furthermore, please execute gradle check to check that your changes don't break our build which seems to be the case:

:checkstyleJmh FAILED

FAILURE: Build failed with an exception.

* What went wrong:
Execution failed for task ':checkstyleJmh'.
> Unable to create a Checker: configLocation {/tmp/checkstyle/config/checkstyle/checkstyle.xml},

private static final Pattern HTML_NEWLINE = Pattern.compile(" ?<br>|<BR>");
private static final Pattern REMOVE_HTML = Pattern.compile("<(?!br)(?!BR).*?>");
private static final Pattern WHITESPACE_AROUND_NEWLINE = Pattern.compile("(?m)^\\s+|\\v+");
private static final Pattern DOUBLE_WHITESPACES = Pattern.compile("[ ]{2,}");
private final String htmlText;

Copy link
Member

Choose a reason for hiding this comment

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

I don't see why there should be a blank line

@koppor koppor changed the base branch from checkstyle to master April 20, 2017 09:07
@koppor koppor removed the status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers label Apr 20, 2017
…into checkstyle

# Conflicts:
#	src/main/java/org/jabref/gui/entryeditor/FieldExtraComponents.java
#	src/main/java/org/jabref/preferences/JabRefPreferences.java
@LinusDietz LinusDietz added the status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers label Apr 21, 2017
@LinusDietz LinusDietz changed the title Clean up checkstyle warnings [WIP] Clean up checkstyle warnings Apr 21, 2017
@LinusDietz LinusDietz removed the status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers label Apr 21, 2017
@LinusDietz
Copy link
Member

@snisnisniksonah, I have added a [WIP] tag to the title to indicate that this PR is still work in progress. Please remove it when you want feedback for this PR.

@koppor koppor added the status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers label Apr 21, 2017
@snisnisniksonah snisnisniksonah changed the title [WIP] Clean up checkstyle warnings Clean up checkstyle warnings Apr 21, 2017
Copy link
Member

@LinusDietz LinusDietz left a comment

Choose a reason for hiding this comment

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

I think these changes are fine. Just update the version number of checkstyle and it can be merged.

build.gradle Outdated
@@ -390,7 +390,7 @@ task media(type: com.install4j.gradle.Install4jTask, dependsOn: "releaseJar") {
checkstyle {
// do not use other packages for checkstyle, excluding gen(erated) sources
checkstyleMain.source = "src/main/java"
toolVersion = '6.17'
toolVersion = '7.6'
Copy link
Member

Choose a reason for hiding this comment

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

The latest version is already 7.6.1.

@koppor koppor merged commit ae6d01b into JabRef:master Apr 21, 2017
@koppor koppor removed the status: ready-for-review Pull Requests that are ready to be reviewed by the maintainers label Apr 21, 2017
@koppor
Copy link
Member

koppor commented Apr 26, 2017

@snisnisniksonah Could you try to adapt the IntelliJ code style accordingly (https://github.com/JabRef/jabref/tree/master/config)?

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.

4 participants