Checkstyle: enforce separators' placement in line wraps #319 #368

eugenepeh · 2017-03-28T01:15:26Z

Addition to #319

mention-bot · 2017-03-28T01:15:27Z

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

CanIHasReview-bot · 2017-03-28T01:27:02Z

v1

@eugenepeh submitted v1 for review.

[v1 1/1] Checkstyle: add SeparatorWrap rules

(📚 Archive)

Checkout this PR version locally

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

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

PierceAndy

[v1 1/1] Commit message:

Thus it should be consistent and follow
having the comma (,) stay
having the dot (.) follow
It will ensure
Lines are wrapping at inconsistent and odd positions.
While your commit messages are generally quite good, minor nits remain (e.g. the first paragraph should be about the current situation). Please refer to our most up-to-date guidelines on commit messages. I understand that you might have missed these updated guidelines as they are still in its final stages of PR reviewing.

chao1995

According to our coding standard on line wrapping (item 3 under Files), we also require rules like Break before an operator. Thus I think checkstyle's separatorWrap rule looks like a better fit. But I think we still need to tinker with it.

eugenepeh · 2017-03-29T06:23:22Z

Thanks for pointing that out @chao1995, I have missed out some of it.
The operators though, it seems like from checkstyle documentation
(http://checkstyle.sourceforge.net/config_whitespace.html#SeparatorWrap),
doesn't seems available yet in token list.

Should we still proceed with what we can do first, while submitting a ticket to checkstyle,
and hope that it will be implemented in future?

PierceAndy · 2017-03-29T06:45:38Z

@chao1995 Did you mean OperatorWrap?

@eugenepeh This would warrant its own PR + updates to #319. This PR can proceed as they deal with different sets of characters.

CanIHasReview-bot · 2017-03-29T06:57:20Z

v2

@eugenepeh submitted v2 for review.

[v2 1/1] Checkstyle: add SeparatorWrap rules

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

Checkout this PR version locally

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

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

chao1995 · 2017-03-29T06:58:00Z

@eugenepeh

The operators though, it seems like from checkstyle documentation
(http://checkstyle.sourceforge.net/config_whitespace.html#SeparatorWrap),
doesn't seems available yet in token list.

The one you manage to find actually is OperatorWrap as pointed out by @PierceAndy, which should be done in a separate PR. Sorry for the bad example.

But if there are some other checks supported by separatorWrapRule, which happens to also be our line wrapping rule, e.g. A method or constructor name stays attached to the open parenthesis ( that follows it. (checkstyle LPAREN), we should add that to this PR.

PierceAndy · 2017-03-29T08:23:09Z

config/checkstyle/checkstyle.xml

+      <property name="tokens" value="ELLIPSIS"/>
+      <property name="tokens" value="SEMI"/>
+      <property name="tokens" value="LPAREN"/>
+      <property name="option" value="EOL"/>


Default value. Can be omitted.

PierceAndy · 2017-03-29T08:24:30Z

config/checkstyle/checkstyle.xml

+    <module name="SeparatorWrap">
+      <!-- Checks that the ".", "at" is at the next line in a line wrap. -->
+      <property name="tokens" value="DOT"/>
+      <property name="tokens" value="AT"/>


I believe that you can list all tokens property values in a single tokens property by using commas:
<property name="tokens" value="DOT, AT"/>

CanIHasReview-bot · 2017-03-29T16:09:33Z

v3

@eugenepeh submitted v3 for review.

[v3 1/1] Checkstyle: add SeparatorWrap rules

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

Checkout this PR version locally

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

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

PierceAndy

LGTM

chao1995 · 2017-03-30T05:27:51Z

config/checkstyle/checkstyle.xml

+    </module>
+    <module name="SeparatorWrap">
+      <!-- Checks that the ",", "]", "[", "...", ";", "(" is at the previous end of line in a line wrap. -->
+      <property name="tokens" value="COMMA, RBRACK, ARRAY_DECLARATOR, ELLIPSIS, SEMI, LPAREN"/>


even though eol is the default value for option, let's still put it there for clarity reason, since it's not an obvious default.

@PierceAndy, does this conflict with issue #370?

The option property tag isn't necessary as the comment clarifies the policy on how to wrap lines.

We could argue that we should include properties with default values, and not use comments. However to be consistent, we would have to include all properties for all checkstyle modules, as every property has default values. This would unnecessarily add a lot of lines without necessarily enhancing readability, when compared to using comments.

This is a problem of standardization for checkstyle.xml, as we still don't have one.

I think it's better to be consistent with the configuration provided by checkstyle, if there is no strong reason not to do so.

also, value means one value, for multiple values, I would use values=

About the omitting of properties with default values, the configuration provided does so as well.
See: configuration by checkstyle
See: checkstyle docs with all properties

This is the option we are talking about.

Note that there are two separator rules in the config file. And I feel it's strange to have one separator rule has an option "nl", but the other has no option. It's not obvious the option is actually "eol".

Yes, if using the same module multiple times, it seems better to not omit properties with default values if these properties are different across modules with the same name.

chao1995 · 2017-03-30T05:28:51Z

config/checkstyle/checkstyle.xml

+    </module>
+    <module name="SeparatorWrap">
+      <!-- Checks that the ",", "]", "[", "...", ";", "(" is at the previous end of line in a line wrap. -->
+      <property name="tokens" value="COMMA, RBRACK, ARRAY_DECLARATOR, ELLIPSIS, SEMI, LPAREN"/>


I would prefer not to use "," to separate them. Let's follow checkstyle's style, use multiple properties.

This, however, will result in non-standardization though, since all other rules in checkstyle config file have their tokens all in single line.
Is there any guideline regarding this?
Issue oss-generic/process#84 Typo**, @PierceAndy

#84 is not an issue. Typo?

Is there any guideline regarding this?

I think it's better to be consistent with the configuration provided by checkstyle, if there is no strong reason not to do so.

I personally thinks that excessive long modules (similar to methods) do affects readability though, especially module such as OperatorWrap which we should take into consideration as well if we were to implement that in our checkstyle configuration too.

Your thoughts on this, @PierceAndy ?

Yes, it seems it would be better to have a single value per property.

chao1995 · 2017-03-30T06:13:28Z

src/main/java/seedu/address/model/tag/UniqueTagList.java

-                && this.internalList.equals(
-                ((UniqueTagList) other).internalList));
+                && this.internalList.equals(((UniqueTagList) other)
+                        .internalList));


looks like we don't need a line wrap here.

chao1995 · 2017-03-30T06:34:42Z

src/test/java/seedu/address/ui/TestFxmlObject.java

@@ -27,8 +27,7 @@ public void setText(String text) {

    @Override
    public boolean equals(Object other) {
-        return other == this ||
-                (other instanceof TestFxmlObject
+        return other == this || (other instanceof TestFxmlObject


can format similar to UniqueTagList.java above, which is slightly easier to read i.e.

return other == this // short circuit if same object || (other instanceof TestFxmlObject && this.text.equals(((TestFxmlObject) other).getText()));

CanIHasReview-bot · 2017-04-01T08:16:40Z

v4

@eugenepeh submitted v4 for review.

[v4 1/1] Checkstyle: add SeparatorWrap rules

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

Checkout this PR version locally

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

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

damithc · 2017-04-01T09:13:30Z

src/test/java/seedu/address/ui/TestFxmlObject.java

-                        && this.text.equals(((TestFxmlObject) other).getText()));
+        return other == this // short circuit if same object
+                || (other instanceof TestFxmlObject
+                && this.text.equals(((TestFxmlObject) other).getText()));


This however is misleading: || and && are operating at different levels in the expression while this line wrapping gives the impression they are at the same level.

Does the code below violate any of our rules/standards? I remember we decided not to have custom indentation levels but we may have to make an exception for expressions.

return other == this // short circuit if same object || (other instanceof TestFxmlObject && this.text.equals(((TestFxmlObject) other).getText()));

chao1995 · 2017-04-01T13:35:20Z

src/main/java/seedu/address/model/person/UniquePersonList.java

@@ -103,8 +103,7 @@ public void setPersons(List<? extends ReadOnlyPerson> persons) throws DuplicateP
    public boolean equals(Object other) {
        return other == this // short circuit if same object
                || (other instanceof UniquePersonList // instanceof handles nulls
-                && this.internalList.equals(
-                ((UniquePersonList) other).internalList));
+                && this.internalList.equals(((UniquePersonList) other).internalList));


Please indent this line as well.

CanIHasReview-bot · 2017-04-04T03:26:55Z

v5

@eugenepeh submitted v5 for review.

[v5 1/1] Checkstyle: add SeparatorWrap rules

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

Checkout this PR version locally

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

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

chao1995 · 2017-04-04T04:27:12Z

config/checkstyle/checkstyle.xml

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

+    <module name="SeparatorWrap">
+      <!-- Checks that the ".", "at" is at the next line in a line wrap. -->
+      <property name="tokens" value="DOT, AT"/>


@damithc Prof, what is your thought of using a "," to separate values rather than using multiple properties with each line having one single value, like this:

<property name="tokens" value="DOT"/> <property name="tokens" value="AT"/>

Our previous discussion here.

We came to the consensus that for readability purposes, it would be better to have one value per property tag.

While I'm ok with one value per line, it looks a bit odd when the same property is being assigned multiple times, especially when the property is supposed to be multi-valued. It looks almost like the value is being overwritten multiple times.

It looks almost like the value is being overwritten multiple times.

@eugenepeh Prof @damithc pointed out something I didn't think of before. Indeed it can be misleading, so I'm fine with the current changes.

chao1995 · 2017-04-04T19:23:07Z

Some suggestions to the commit message

Our coding standard for Java requires that line wrappings that
involves separator to follow a specific format, such as having the
comma (,) stay attached to the token that precedes it and having the
dot (.) follow the symbol in next line. This rule is not enforced by
checkstyle.

requires ~~that~~ line wrappings
involve separators
a specific format to specific rules
~~the~~ comma
the dot (.) follow the symbol in next line to dot (.) in a new line
This rule is to These rules are.

A code example would be like this
s
.isEmpty();
foo(i,
s);

These texts are not very nicely formatted. Since we have explained this example in the previous paragraph, I think we can leave the example out.

Let's fix this by adding SeparatorWrap rules to ensure that our line
wrappings, that involve separators, are standardized and consistent.

are standardized and consistent to are consistent with our coding standard.

CanIHasReview-bot · 2017-04-05T03:29:51Z

v6

@eugenepeh submitted v6 for review.

[v6 1/1] Checkstyle: add SeparatorWrap rules

(📚 Archive) (📈 Interdiff between v5 and v6)

Checkout this PR version locally

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

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

PierceAndy

Just one last minor thing.

PierceAndy · 2017-04-05T17:52:26Z

config/checkstyle/checkstyle.xml

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

+    <module name="SeparatorWrap">
+      <!-- Checks that the ".", "at" is at the next line in a line wrap. -->


To be consistent with the others:
"at" -> "@"

CanIHasReview-bot · 2017-04-06T11:17:37Z

v7

@eugenepeh submitted v7 for review.

[v7 1/1] Checkstyle: add SeparatorWrap rules

(📚 Archive) (📈 Interdiff between v6 and v7)

Checkout this PR version locally

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

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

Our coding standard for Java requires separators' placement in line wraps to follow specific rules, such as having comma (,) stay attached to the token that precedes it and having the dot (.) in a new line. These rules are not enforced by checkstyle. Let's teach Checkstyle to enforce the coding standard above by adding the SeparatorWrap rules.

CanIHasReview-bot · 2017-04-11T11:27:31Z

v8

@eugenepeh submitted v8 for review.

[v8 1/1] Checkstyle: enforce separators' placement in line wraps

(📚 Archive) (📈 Interdiff between v7 and v8)

Checkout this PR version locally

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

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

eugenepeh changed the title ~~Checkstyle: add SeparatorWrap rules~~ Checkstyle: add SeparatorWrap rules #319 Mar 28, 2017

CanIHasReview-bot added the s.ToReview label Mar 28, 2017

PierceAndy suggested changes Mar 28, 2017

View reviewed changes

PierceAndy added s.Ongoing and removed s.ToReview labels Mar 28, 2017

chao1995 suggested changes Mar 28, 2017

View reviewed changes

CanIHasReview-bot added s.ToReview and removed s.Ongoing labels Mar 29, 2017

CanIHasReview-bot requested review from chao1995 and PierceAndy March 29, 2017 06:57

PierceAndy added s.Ongoing and removed s.ToReview labels Mar 29, 2017

PierceAndy suggested changes Mar 29, 2017

View reviewed changes

CanIHasReview-bot added s.ToReview and removed s.Ongoing labels Mar 29, 2017

CanIHasReview-bot requested a review from PierceAndy March 29, 2017 16:09

PierceAndy approved these changes Mar 29, 2017

View reviewed changes

chao1995 suggested changes Mar 30, 2017

View reviewed changes

PierceAndy mentioned this pull request Mar 30, 2017

Add XML coding standards oss-generic/process#84

Open

yamgent added s.Ongoing and removed s.ToReview labels Mar 30, 2017

CanIHasReview-bot removed the s.Ongoing label Apr 1, 2017

damithc reviewed Apr 1, 2017

View reviewed changes

chao1995 reviewed Apr 1, 2017

View reviewed changes

CanIHasReview-bot requested a review from chao1995 April 4, 2017 03:26

chao1995 reviewed Apr 4, 2017

View reviewed changes

chao1995 added s.Ongoing and removed s.ToReview labels Apr 4, 2017

CanIHasReview-bot added s.ToReview and removed s.Ongoing labels Apr 5, 2017

CanIHasReview-bot requested a review from chao1995 April 5, 2017 03:29

chao1995 approved these changes Apr 5, 2017

View reviewed changes

chao1995 requested review from damithc and PierceAndy April 5, 2017 06:09

PierceAndy suggested changes Apr 5, 2017

View reviewed changes

CanIHasReview-bot requested a review from PierceAndy April 6, 2017 11:17

PierceAndy approved these changes Apr 6, 2017

View reviewed changes

eugenepeh changed the title ~~Checkstyle: add SeparatorWrap rules #319~~ Checkstyle: enforce separators consistency in line wraps Apr 8, 2017

eugenepeh changed the title ~~Checkstyle: enforce separators consistency in line wraps~~ Checkstyle: enforce separators consistency in line wraps #319 Apr 8, 2017

eugenepeh changed the title ~~Checkstyle: enforce separators consistency in line wraps #319~~ Checkstyle: enforce separators' placement in line wraps #319 Apr 11, 2017

PierceAndy added s.ToMerge and removed s.ToReview labels Apr 11, 2017

damithc approved these changes Apr 14, 2017

View reviewed changes

damithc merged commit dc9fe96 into se-edu:master Apr 14, 2017

eugenepeh deleted the 319-add-SeparatorWrap branch April 14, 2017 16:20

eugenepeh mentioned this pull request Apr 18, 2017

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

Merged

Checkstyle: enforce separators' placement in line wraps #319 #368

Checkstyle: enforce separators' placement in line wraps #319 #368

Conversation

eugenepeh commented Mar 28, 2017 • edited Loading

mention-bot commented Mar 28, 2017

CanIHasReview-bot commented Mar 28, 2017

v1

PierceAndy left a comment

Choose a reason for hiding this comment

chao1995 left a comment • edited Loading

Choose a reason for hiding this comment

eugenepeh commented Mar 29, 2017 • edited Loading

PierceAndy commented Mar 29, 2017 • edited Loading

CanIHasReview-bot commented Mar 29, 2017

v2

chao1995 commented Mar 29, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CanIHasReview-bot commented Mar 29, 2017

v3

PierceAndy left a comment

Choose a reason for hiding this comment

chao1995 Mar 30, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PierceAndy Mar 30, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

PierceAndy Mar 30, 2017 • edited Loading

Choose a reason for hiding this comment

chao1995 Mar 30, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

eugenepeh Mar 30, 2017 • edited Loading

Choose a reason for hiding this comment

PierceAndy Mar 30, 2017 • edited Loading

Choose a reason for hiding this comment

chao1995 Mar 30, 2017 • edited Loading

Choose a reason for hiding this comment

eugenepeh Mar 30, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CanIHasReview-bot commented Apr 1, 2017

v4

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CanIHasReview-bot commented Apr 4, 2017

v5

chao1995 Apr 4, 2017 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

chao1995 commented Apr 4, 2017

CanIHasReview-bot commented Apr 5, 2017

v6

PierceAndy left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

CanIHasReview-bot commented Apr 6, 2017

v7

CanIHasReview-bot commented Apr 11, 2017

v8

eugenepeh commented Mar 28, 2017 •

edited

Loading

chao1995 left a comment •

edited

Loading

eugenepeh commented Mar 29, 2017 •

edited

Loading

PierceAndy commented Mar 29, 2017 •

edited

Loading

chao1995 commented Mar 29, 2017 •

edited

Loading

chao1995 Mar 30, 2017 •

edited

Loading

PierceAndy Mar 30, 2017 •

edited

Loading

PierceAndy Mar 30, 2017 •

edited

Loading

chao1995 Mar 30, 2017 •

edited

Loading

eugenepeh Mar 30, 2017 •

edited

Loading

PierceAndy Mar 30, 2017 •

edited

Loading

chao1995 Mar 30, 2017 •

edited

Loading

eugenepeh Mar 30, 2017 •

edited

Loading

chao1995 Apr 4, 2017 •

edited

Loading