ArrayTrailingComma: Extra coma is required in multiline array value #1509

Closed
romani opened this Issue Jul 29, 2015 · 10 comments

Comments

Projects
None yet
5 participants
@romani
Member

romani commented Jul 29, 2015

from MainTest.java of checkstyle source

        // we just reference there all violations
        final String[][] outputValues = new String[][]{
            {"ClassCouplingCheckTestInput", "InnerClass", "7:19"},
            {"BooleanExpressionComplexityCheckTestInput",
                "BooleanExpressionComplexityCheckTestInput", "3:14", },
            {"BooleanExpressionComplexityCheckTestInput", "Settings", "53:19"},
        };

veird part is ""3:14", }," checkstyle demand that coma.
Config is checkstyle_checks.xml

@Vladlis

This comment has been minimized.

Show comment
Hide comment
@Vladlis

Vladlis Sep 4, 2015

Member

As it can be seen from test, this behaviour is expected: input and test.
From documentation:

The check allows leaving out the comma at the end if both the left and right curly brackets are on the same line.

the same refers to the array values - if they are multiline, they should contain a trailing comma.

Member

Vladlis commented Sep 4, 2015

As it can be seen from test, this behaviour is expected: input and test.
From documentation:

The check allows leaving out the comma at the end if both the left and right curly brackets are on the same line.

the same refers to the array values - if they are multiline, they should contain a trailing comma.

@romani romani closed this Sep 4, 2015

@mkordas

This comment has been minimized.

Show comment
Hide comment
@mkordas

mkordas Sep 4, 2015

Contributor

For me it is still not intuitive. Training comma brings value usually only if both braces are on different lines.

My proposition is to change the rule to behave as follows:
The check allows leaving out the comma at the end if either the left or right curly brackets are on the same line.

Contributor

mkordas commented Sep 4, 2015

For me it is still not intuitive. Training comma brings value usually only if both braces are on different lines.

My proposition is to change the rule to behave as follows:
The check allows leaving out the comma at the end if either the left or right curly brackets are on the same line.

@mkordas mkordas reopened this Sep 4, 2015

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Sep 5, 2015

Member

Intention of this rule is not clear for me at all, but that logic exists for many years and nobody complained about that, and it is clearly documented. That is why I decided to close that issue.

I could connect to author of this update, to get from him his motivation of doing that.

Member

romani commented Sep 5, 2015

Intention of this rule is not clear for me at all, but that logic exists for many years and nobody complained about that, and it is clearly documented. That is why I decided to close that issue.

I could connect to author of this update, to get from him his motivation of doing that.

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Sep 5, 2015

Member

The check allows leaving out the comma at the end if either the left or right curly brackets are on the same line.

I do not understand meaning of that rule, please rephrase for me and lets start discussion by code examples (with violation and not)

Member

romani commented Sep 5, 2015

The check allows leaving out the comma at the end if either the left or right curly brackets are on the same line.

I do not understand meaning of that rule, please rephrase for me and lets start discussion by code examples (with violation and not)

@mkordas

This comment has been minimized.

Show comment
Hide comment
@mkordas

mkordas Sep 5, 2015

Contributor

@romani - sure.

Main benefit of training comma is that when you add new entry to array, no surrounding lines are changed:

{
    100000000000000000000, 
    200000000000000000000, // OK
}
{
    100000000000000000000,
    200000000000000000000,
    300000000000000000000,  // Just this line added, no other changes
}

If closing brace is on the same line as training comma, this benefit is gone:

{100000000000000000000, 
    200000000000000000000,} // Trailing comma not needed, line needs to be modified anyway
{100000000000000000000, 
    200000000000000000000, // Modified line
    300000000000000000000,} // Added line

If opening brace is on the same line as training comma there's also (more arguable) problem:

{100000000000000000000, // Line cannot be just duplicated to slightly modify entry
} 
{100000000000000000000,
    100000000000000000001, // More work needed to duplicate
} 
Contributor

mkordas commented Sep 5, 2015

@romani - sure.

Main benefit of training comma is that when you add new entry to array, no surrounding lines are changed:

{
    100000000000000000000, 
    200000000000000000000, // OK
}
{
    100000000000000000000,
    200000000000000000000,
    300000000000000000000,  // Just this line added, no other changes
}

If closing brace is on the same line as training comma, this benefit is gone:

{100000000000000000000, 
    200000000000000000000,} // Trailing comma not needed, line needs to be modified anyway
{100000000000000000000, 
    200000000000000000000, // Modified line
    300000000000000000000,} // Added line

If opening brace is on the same line as training comma there's also (more arguable) problem:

{100000000000000000000, // Line cannot be just duplicated to slightly modify entry
} 
{100000000000000000000,
    100000000000000000001, // More work needed to duplicate
} 
@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Sep 12, 2015

Member

@mkordas

If opening brace is on the same line as training comma there's also (more arguable) problem:

I do not see changes to first line, I do not understand how "{" location affect the rule for "," presence. Please extend examples for this case.

Member

romani commented Sep 12, 2015

@mkordas

If opening brace is on the same line as training comma there's also (more arguable) problem:

I do not see changes to first line, I do not understand how "{" location affect the rule for "," presence. Please extend examples for this case.

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Sep 12, 2015

Member

@os97673 , looks like you were an original author or this rule https://github.com/checkstyle/checkstyle/blame/master/src/main/java/com/puppycrawl/tools/checkstyle/checks/coding/ArrayTrailingCommaCheck.java#L25 , it would be so kind if you can bring from a history, of 2003 year :) , why "," is required (please see issue description).

Member

romani commented Sep 12, 2015

@os97673 , looks like you were an original author or this rule https://github.com/checkstyle/checkstyle/blame/master/src/main/java/com/puppycrawl/tools/checkstyle/checks/coding/ArrayTrailingCommaCheck.java#L25 , it would be so kind if you can bring from a history, of 2003 year :) , why "," is required (please see issue description).

@os97673

This comment has been minimized.

Show comment
Hide comment
@os97673

os97673 Sep 12, 2015

Contributor

Well, the check is supposed to be used when you either write one-line arrays

{ 1, 2, 3}

or place keep curly braces on separate lines and one element per line

{
  1,
  2,
  3,
}

if you do not require this then the check is (almost) useless.
So the check should be always used in combination with another one (for array formatting).
Thus I'd keep the check as is and do suggest do not use it in some cases :)
Hope I've answered the question ;)

Contributor

os97673 commented Sep 12, 2015

Well, the check is supposed to be used when you either write one-line arrays

{ 1, 2, 3}

or place keep curly braces on separate lines and one element per line

{
  1,
  2,
  3,
}

if you do not require this then the check is (almost) useless.
So the check should be always used in combination with another one (for array formatting).
Thus I'd keep the check as is and do suggest do not use it in some cases :)
Hope I've answered the question ;)

@mkordas

This comment has been minimized.

Show comment
Hide comment
@mkordas

mkordas Sep 14, 2015

Contributor

@romani

I do not see changes to first line, I do not understand how "{" location affect the rule for "," presence. Please extend examples for this case.

Example I've given should be enough. If { is on the same line as , you have more work to duplicate the line. Instead of duplicating and changing just value, you also need to remove { from duplicated line.

Contributor

mkordas commented Sep 14, 2015

@romani

I do not see changes to first line, I do not understand how "{" location affect the rule for "," presence. Please extend examples for this case.

Example I've given should be enough. If { is on the same line as , you have more work to duplicate the line. Instead of duplicating and changing just value, you also need to remove { from duplicated line.

Vladlis added a commit to Vladlis/checkstyle that referenced this issue Apr 11, 2017

Vladlis added a commit to Vladlis/checkstyle that referenced this issue Apr 11, 2017

Vladlis added a commit to Vladlis/checkstyle that referenced this issue Apr 12, 2017

Vladlis added a commit to Vladlis/checkstyle that referenced this issue Apr 12, 2017

Vladlis added a commit to Vladlis/checkstyle that referenced this issue Apr 12, 2017

Vladlis added a commit to Vladlis/checkstyle that referenced this issue Apr 14, 2017

Vladlis added a commit to Vladlis/checkstyle that referenced this issue Apr 15, 2017

@romani romani changed the title from Extra coma is required in multiline array value to ArrayTrailingComma: Extra coma is required in multiline array value Apr 17, 2017

Vladlis added a commit to Vladlis/checkstyle that referenced this issue Apr 17, 2017

Vladlis added a commit to Vladlis/checkstyle that referenced this issue Apr 18, 2017

Vladlis added a commit to Vladlis/checkstyle that referenced this issue Apr 18, 2017

Vladlis added a commit to Vladlis/checkstyle that referenced this issue Apr 18, 2017

Vladlis added a commit to Vladlis/checkstyle that referenced this issue Apr 19, 2017

Vladlis added a commit to Vladlis/checkstyle that referenced this issue Apr 20, 2017

Vladlis added a commit to Vladlis/checkstyle that referenced this issue Apr 20, 2017

rnveach added a commit that referenced this issue Apr 22, 2017

@rnveach

This comment has been minimized.

Show comment
Hide comment
@rnveach

rnveach Apr 22, 2017

Member

Fix is merged

Member

rnveach commented Apr 22, 2017

Fix is merged

@rnveach rnveach closed this Apr 22, 2017

@rnveach rnveach added this to the 7.7 milestone Apr 22, 2017

@romani romani added the bug label Apr 22, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment