Checkstyle: enforce operators' placement in line wraps #319 #382

eugenepeh · 2017-04-18T15:45:38Z

Addition to #319
Enhance #368

mention-bot · 2017-04-18T15:45:39Z

@eugenepeh, thanks for your PR! By analyzing the history of the files in this pull request, we identified @yl-coder, @damithc and @m133225 to be potential reviewers.

yamgent · 2017-05-02T01:33:48Z

@eugenepeh Sorry was the PR supposed to be ready? The PR does not have a CanIHasReview posted to it.

eugenepeh · 2017-05-02T10:41:02Z

@yamgent so sorry was a little busy with finals.
Its not ready yet, there's some error in the build, I will post a CanIHasReview as soon as I fix it.

CanIHasReview-bot · 2017-05-10T15:06:03Z

v1

@eugenepeh submitted v1 for review.

[v1 1/1] Checkstyle: enforce operators' placement in line wraps

(📚 Archive)

Checkout this PR version locally

git fetch https://github.com/se-edu/addressbook-level4.git refs/pr/382/1/head:BRANCHNAME

where BRANCHNAME is the name of the local branch you want to fetch this PR to.

yamgent

I can't get this to work on my side. The following errors appear. The errors disappear when I deleted the lines that you added:

Executing external tasks 'checkstyleMain checkstyleTest'...
:compileJava UP-TO-DATE
:processResources UP-TO-DATE
:classes UP-TO-DATE
:checkstyleMain FAILED

FAILURE: Build failed with an exception.

* What went wrong:
Execution failed for task ':checkstyleMain'.
> Unable to create a Checker: configLocation {/home/wangleng/github/yamgent/addressbook-level4/config/checkstyle/checkstyle.xml}, classpath {/home/wangleng/github/yamgent/addressbook-level4/build/classes/main:/home/wangleng/github/yamgent/addressbook-level4/build/resources/main}.

* Try:
Run with --stacktrace option to get the stack trace. Run with --info or --debug option to get more log output.

BUILD FAILED

Total time: 0.866 secs
The content of element type "module" must match "(module|property|metadata|message)*".
8:24:19 AM: External tasks execution finished 'checkstyleMain checkstyleTest'.

yamgent · 2017-05-11T00:28:20Z

config/checkstyle/checkstyle.xml

+    </module>
+    <module name="OperatorWrap">
+      <!-- Checks that assign type operator is at the previous end of line in a line wrap.
+           This include "=", "/=", "+=", "-=", "*=", "%=", ">>=", ">>>=", "<<=", "^=", "&=".


Unless we are using default values (i.e. not specifying the value property manually), the constants ASSIGN, DIV_ASSIGN, etc... seems to be self explanatory. Not sure whether we need to repeat it in the comments in such cases.

Ahh... I added them in comments because some values can be hard to understand without referring to the checkstyle documentation. Also, it would be weird to add just some of them, so I added them all. Might need more opinions on this.

P.S
Thanks for testing it on your side because I been trying to resolve this weird error.

eugenepeh · 2017-05-13T15:29:20Z

build.gradle

@@ -42,7 +42,7 @@ allprojects {
        junitVersion = '4.12'
        testFxVersion = '4.0.5-alpha'
        monocleVersion = '1.8.0_20'
-        checkstyleVersion = '7.1.2'
+        checkstyleVersion = '7.2'


Discovered that the METHOD_REF token needs checkstyle 7.2 to work,
http://checkstyle.sourceforge.net/releasenotes.html#Release_7.2

Excellent, it works now. 👍

CanIHasReview-bot · 2017-05-13T17:11:42Z

v2

@eugenepeh submitted v2 for review.

[v2 1/1] Checkstyle: enforce operators' placement in line wraps

(📚 Archive) (📈 Interdiff between v1 and v2)

Checkout this PR version locally

git fetch https://github.com/se-edu/addressbook-level4.git refs/pr/382/2/head:BRANCHNAME

where BRANCHNAME is the name of the local branch you want to fetch this PR to.

yamgent

Looks good to me, just minor nit about the spellings in the comments.

yamgent · 2017-05-15T01:04:59Z

config/checkstyle/checkstyle.xml

@@ -322,6 +322,28 @@
      <property name="message" value="Trailing whitespace"/>
    </module>

+    <module name="OperatorWrap">
+      <!-- Checks that non-assign type operator is at the next line in a line wrap.


assign -> assignment?

yamgent · 2017-05-15T01:05:21Z

config/checkstyle/checkstyle.xml

+      <!-- Checks that non-assign type operator is at the next line in a line wrap.
+           This include "?", ":", "==", "!=", "/", "+", "-", "*", "%", ">>", ">>>",
+           ">=", ">", "<<", "<=", "<", "^", "|", "||", "&", "&&", "instanceof",
+           "&" when used in a generic upper or lower bounds constrain,


constrain -> constraints?

CanIHasReview-bot · 2017-05-15T14:28:46Z

v3

@eugenepeh submitted v3 for review.

[v3 1/1] Checkstyle: enforce operators' placement in line wraps

(📚 Archive) (📈 Interdiff between v2 and v3)

Checkout this PR version locally

git fetch https://github.com/se-edu/addressbook-level4.git refs/pr/382/3/head:BRANCHNAME

where BRANCHNAME is the name of the local branch you want to fetch this PR to.

yamgent

wraps to follow specific rules, such as having assign operator (=)

The commit message still has the old spelling :P
Should be:

...such as having the assignment operator (=)

yamgent · 2017-05-16T03:12:16Z

config/checkstyle/checkstyle.xml

@@ -322,6 +322,28 @@
      <property name="message" value="Trailing whitespace"/>
    </module>

+    <module name="OperatorWrap">
+      <!-- Checks that non-assignment type operator is at the next line in a line wrap.


Minor grammar nit:

Checks that the non-assignment...

yamgent · 2017-05-16T03:12:33Z

config/checkstyle/checkstyle.xml

@@ -322,6 +322,28 @@
      <property name="message" value="Trailing whitespace"/>
    </module>

+    <module name="OperatorWrap">
+      <!-- Checks that non-assignment type operator is at the next line in a line wrap.
+           This include "?", ":", "==", "!=", "/", "+", "-", "*", "%", ">>", ">>>",


Minor grammar nit:

This includes ...

yamgent · 2017-05-16T03:13:12Z

config/checkstyle/checkstyle.xml

+    </module>
+    <module name="OperatorWrap">
+      <!-- Checks that assignment type operator is at the previous end of line in a line wrap.
+           This include "=", "/=", "+=", "-=", "*=", "%=", ">>=", ">>>=", "<<=", "^=", "&=".


The grammar nit comments can also be applied here.

Our coding standard for Java requires operators' placement in line wraps to follow specific rules, such as having assignment operator (=) stay attached to the token that precedes it and having the arithmetric operator (+) in a new line. These rules are not enforced by checkstyle. Let's teach Checkstyle to be stricter about line wrapping around more symbols by adding the relevant OperatorWrap rules.

CanIHasReview-bot · 2017-05-18T11:33:53Z

v4

@eugenepeh submitted v4 for review.

[v4 1/1] Checkstyle: enforce operators' placement in line wraps

(📚 Archive) (📈 Interdiff between v3 and v4)

Checkout this PR version locally

git fetch https://github.com/se-edu/addressbook-level4.git refs/pr/382/4/head:BRANCHNAME

where BRANCHNAME is the name of the local branch you want to fetch this PR to.

chao1995

Looks good!

yamgent added the s.Ongoing label May 2, 2017

CanIHasReview-bot added s.ToReview and removed s.Ongoing labels May 10, 2017

yamgent reviewed May 11, 2017

View reviewed changes

yamgent added s.Ongoing and removed s.ToReview labels May 11, 2017

eugenepeh commented May 13, 2017

View reviewed changes

CanIHasReview-bot added s.ToReview and removed s.Ongoing labels May 13, 2017

yamgent reviewed May 15, 2017

View reviewed changes

yamgent added s.Ongoing and removed s.ToReview labels May 15, 2017

CanIHasReview-bot added s.ToReview and removed s.Ongoing labels May 15, 2017

yamgent suggested changes May 16, 2017

View reviewed changes

yamgent added s.Ongoing and removed s.ToReview labels May 16, 2017

eugenepeh changed the title ~~Checkstyle: enforce operators' placement in line wraps~~ Checkstyle: enforce operators' placement in line wraps #319 May 17, 2017

CanIHasReview-bot added s.ToReview and removed s.Ongoing labels May 18, 2017

CanIHasReview-bot requested a review from yamgent May 18, 2017 11:33

yamgent approved these changes May 19, 2017

View reviewed changes

yamgent requested review from damithc and chao1995 May 19, 2017 00:23

chao1995 approved these changes May 19, 2017

View reviewed changes

damithc approved these changes May 19, 2017

View reviewed changes

yamgent added s.ToMerge and removed s.ToReview labels May 19, 2017

yamgent merged commit cc22ed6 into se-edu:master May 22, 2017

eugenepeh deleted the 319-add-OperatorWrap-rules branch August 18, 2017 08:59

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Checkstyle: enforce operators' placement in line wraps #319 #382

Checkstyle: enforce operators' placement in line wraps #319 #382

eugenepeh commented Apr 18, 2017

mention-bot commented Apr 18, 2017

yamgent commented May 2, 2017

eugenepeh commented May 2, 2017

CanIHasReview-bot commented May 10, 2017

yamgent left a comment

yamgent May 11, 2017

eugenepeh May 11, 2017 •

edited

Loading

eugenepeh May 13, 2017 •

edited

Loading

yamgent May 15, 2017

CanIHasReview-bot commented May 13, 2017

yamgent left a comment

yamgent May 15, 2017

yamgent May 15, 2017

CanIHasReview-bot commented May 15, 2017

yamgent left a comment

yamgent May 16, 2017

yamgent May 16, 2017

yamgent May 16, 2017 •

edited

Loading

CanIHasReview-bot commented May 18, 2017

chao1995 left a comment

Checkstyle: enforce operators' placement in line wraps #319 #382

Checkstyle: enforce operators' placement in line wraps #319 #382

Conversation

eugenepeh commented Apr 18, 2017

mention-bot commented Apr 18, 2017

yamgent commented May 2, 2017

eugenepeh commented May 2, 2017

CanIHasReview-bot commented May 10, 2017

v1

yamgent left a comment

Choose a reason for hiding this comment

yamgent May 11, 2017

Choose a reason for hiding this comment

eugenepeh May 11, 2017 • edited Loading

Choose a reason for hiding this comment

eugenepeh May 13, 2017 • edited Loading

Choose a reason for hiding this comment

yamgent May 15, 2017

Choose a reason for hiding this comment

CanIHasReview-bot commented May 13, 2017

v2

yamgent left a comment

Choose a reason for hiding this comment

yamgent May 15, 2017

Choose a reason for hiding this comment

yamgent May 15, 2017

Choose a reason for hiding this comment

CanIHasReview-bot commented May 15, 2017

v3

yamgent left a comment

Choose a reason for hiding this comment

yamgent May 16, 2017

Choose a reason for hiding this comment

yamgent May 16, 2017

Choose a reason for hiding this comment

yamgent May 16, 2017 • edited Loading

Choose a reason for hiding this comment

CanIHasReview-bot commented May 18, 2017

v4

chao1995 left a comment

Choose a reason for hiding this comment

eugenepeh May 11, 2017 •

edited

Loading

eugenepeh May 13, 2017 •

edited

Loading

yamgent May 16, 2017 •

edited

Loading