Issue #6971: Added implementation for NoArrayTrailingCommaCheck by shashvat-kedia · Pull Request #7228 · checkstyle/checkstyle

shashvat-kedia · 2019-10-27T15:12:58Z

Fix for issue #6971.

Contribution PR: checkstyle/contribution#436

Regression report:
https://sd1998.github.io/checkstyle-regression/Fix-6971/index.html

Base config:
https://sd1998.github.io/checkstyle-regression/Fix-6971/base_config.xml

Patch config:
https://sd1998.github.io/checkstyle-regression/Fix-6971/patch_config.xml

Report repo:
https://github.com/sd1998/checkstyle-regression/tree/master/Fix-6971

shashvat-kedia · 2019-10-27T15:13:31Z

@romani I have added an implementation of NoArrayTrailingCommaCheck.

rnveach · 2019-10-27T15:31:23Z

@SD1998 Thanks. It is best we try to finish the already opened PRs. One is waiting on CI to pass.

romani

Please review failures in Teamcity CI (you can login as guest)
Please review Travis CI for failures, please address them.
In Travis there should be only one failure - at ensure that all modules are used in no exception configs
but please send PR to contribution repo also, we will merge this PR and contribution PR together. Please share link to PR in this PR description.

items to improve:

romani · 2019-11-18T17:30:15Z

@SD1998 , CI need to be green to let me merge,
https://travis-ci.org/checkstyle/checkstyle/jobs/611305595#L1205 do you have problems to resolve this ?
All Checks need to be active in our config, this Check with ignore severity, as it confly with another Check.

Only one CI failure is expected - about new Check presence in contribution config, you did checkstyle/contribution#417

romani

See comment above

In addition .... New Checks need to make sure they are compliant with xpath suppression, please add test to https://github.com/checkstyle/checkstyle/tree/master/src/it/java/org/checkstyle/suppressionxpathfilter

romani · 2019-12-02T05:35:17Z

@SD1998 , please rebase and resolve conflict.

please add xpath tests.

romani · 2019-12-12T18:50:17Z

@SD1998 , ping.

shashvat-kedia · 2019-12-17T11:39:24Z

@romani runVerifications method in AbstractXpathTestSupport.java tries to extract the row number as well as the column number but in NoArrayTrailingCommaCheck.java the error message only comprises of the row no. (so that it is consistent with what is done in ArrayTrailingCommaCheck.java) should I modify the method to extract only the row numbers incase the error message does not contain column numbers?

romani · 2019-12-19T23:55:22Z

yes, looks like we have NO support in Xpath https://github.com/checkstyle/checkstyle/blob/master/src/it/java/org/checkstyle/suppressionxpathfilter/AbstractXpathTestSupport.java#L55 for Checks that report only line number.

It is kind of reasonable as xpath imply detection of exact AST where violation is happening.
Lets report violation on certain line and column.
Extra comma is definitely has line and column so you will place violation on exact symbol that Check do not like - that is good.

[ERROR] AllChecksTest.testAllCheckstyleModulesInCheckstyleConfig:463 checkstyle_checks.xml is missing module: NoArrayTrailingComma ==> expected: but was:

please add Check to checkstyle_checks.xml but with "ignore" severity, in comment above explain that is conflicts with another Check.

[checkstyle] [ERROR] /home/travis/build/checkstyle/checkstyle/src/test/java/com/puppycrawl/tools/checkstyle/checks/coding/NoArrayTrailingCommaCheckTest.java:6:1: Disallowed import - org.junit.Test. [ImportControlTest]

we migrated to Junit5 recently, please use junit 5 types, look at other Tests for examples.

rnveach · 2019-12-20T01:43:30Z

yes, looks like we have NO support in Xpath AbstractXpathTestSupport for Checks that report only line number.

See #5777

shashvat-kedia · 2019-12-24T10:09:12Z

@romani I have made all the changes.

romani · 2019-12-24T10:24:25Z

upps .... I did not notice your comment and pushed all changes myself to your remote branch with force.

Please push one more time from your local. Looks like I did the same code changes as you..... I thought that you abandoned your PR.

shashvat-kedia · 2019-12-24T10:25:32Z

@romani Done.

romani

items to improve:

romani · 2019-12-24T17:02:26Z

Please satisfy spell checker by whitelisting of word https://travis-ci.org/checkstyle/checkstyle/jobs/629058068#L281

Regression report:

I see no violations from your Check.
Please remove from config all Checks that are not your new Check. Please generate report on all projects in property file, we shy have some violations in report.

shashvat-kedia · 2019-12-25T07:02:54Z

@romani I have updated the regression report.

romani

code is good.
report is good.

@rnveach , please do final review.

rnveach · 2019-12-28T15:15:33Z

Contribution PR: checkstyle/contribution#417

CI failed to notice that PR is not for checks-nonjavadoc-error.xml .

romani · 2019-12-28T19:02:54Z

validation is not ideal ....
message is pointing to correct files - https://github.com/checkstyle/checkstyle/blob/master/.ci/travis/travis.sh#L509
so it is just human mistake, confusion file will be removed at checkstyle/contribution#435

romani requested changes Nov 3, 2019

View reviewed changes

Comment thread src/main/java/com/puppycrawl/tools/checkstyle/checks/coding/NoArrayTrailingCommaCheck.java Outdated

Comment thread pom.xml Outdated

Comment thread src/main/resources/com/puppycrawl/tools/checkstyle/checks/coding/messages.properties

romani added the awaiting changes label Nov 3, 2019

shashvat-kedia force-pushed the Fix-6971 branch from f6bebc2 to 2fff8d8 Compare November 6, 2019 05:58

strkkk reviewed Nov 6, 2019

View reviewed changes

Comment thread ...uppycrawl/tools/checkstyle/checks/coding/noarraytrailingcomma/InputNoArrayTrailingComma.java

Comment thread src/xdocs/config_coding.xml Outdated

shashvat-kedia mentioned this pull request Nov 11, 2019

Add NoArrayTrailingCommaCheck checkstyle/contribution#416

Closed

shashvat-kedia force-pushed the Fix-6971 branch 2 times, most recently from 0c49436 to 83dafc6 Compare November 12, 2019 12:32

strkkk approved these changes Nov 12, 2019

View reviewed changes

rnveach assigned romani Nov 12, 2019

rnveach removed the awaiting changes label Nov 12, 2019

shashvat-kedia force-pushed the Fix-6971 branch 3 times, most recently from 9c95592 to 15614f6 Compare November 13, 2019 10:35

romani requested changes Nov 18, 2019

View reviewed changes

romani added the awaiting changes label Dec 2, 2019

romani removed their assignment Dec 2, 2019

romani mentioned this pull request Dec 2, 2019

config: update 'verify-no-exception-configs' validation to not fail build if some contribution PR is referenced in description #7296

Merged

romani force-pushed the Fix-6971 branch from 15614f6 to d548433 Compare December 15, 2019 06:25

shashvat-kedia mentioned this pull request Dec 15, 2019

Fix #416: NoArrayTrailingComma check added to all-checkstyle-checks.xml checkstyle/contribution#417

Closed

shashvat-kedia force-pushed the Fix-6971 branch from d548433 to ad764a0 Compare December 17, 2019 10:12

romani removed the awaiting changes label Dec 17, 2019

romani self-assigned this Dec 19, 2019

shashvat-kedia force-pushed the Fix-6971 branch from ad764a0 to 51a3978 Compare December 24, 2019 10:09

shashvat-kedia force-pushed the Fix-6971 branch from 51a3978 to c19d354 Compare December 24, 2019 10:12

romani force-pushed the Fix-6971 branch from c19d354 to 74607be Compare December 24, 2019 10:21

shashvat-kedia force-pushed the Fix-6971 branch from 74607be to c19d354 Compare December 24, 2019 10:25

romani requested changes Dec 24, 2019

View reviewed changes

shashvat-kedia force-pushed the Fix-6971 branch from c19d354 to 0f94791 Compare December 24, 2019 10:46

pbludov reviewed Dec 24, 2019

View reviewed changes

Comment thread src/xdocs/config_coding.xml

shashvat-kedia force-pushed the Fix-6971 branch from 0f94791 to 88b9702 Compare December 25, 2019 05:14

pbludov approved these changes Dec 25, 2019

View reviewed changes

romani approved these changes Dec 26, 2019

View reviewed changes

romani assigned rnveach and unassigned romani Dec 26, 2019

Issue checkstyle#6971: NoArrayTrailingCommaCheck added

89f393a

shashvat-kedia force-pushed the Fix-6971 branch from 88b9702 to 89f393a Compare December 26, 2019 09:38

rnveach approved these changes Dec 28, 2019

View reviewed changes

romani mentioned this pull request Dec 28, 2019

Issue checkstyle/checkstyle#6971: add NoEnumTrailingComma to checks-nonjavadoc-error.xml checkstyle/contribution#436

Merged

romani merged commit f0af108 into checkstyle:master Dec 28, 2019

Uh oh!

Conversation

shashvat-kedia commented Oct 27, 2019 • edited by romani Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

shashvat-kedia commented Oct 27, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rnveach commented Oct 27, 2019

Uh oh!

romani left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

romani commented Nov 18, 2019

Uh oh!

romani left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

romani commented Dec 2, 2019

Uh oh!

romani commented Dec 12, 2019

Uh oh!

shashvat-kedia commented Dec 17, 2019

Uh oh!

romani commented Dec 19, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rnveach commented Dec 20, 2019

Uh oh!

shashvat-kedia commented Dec 24, 2019

Uh oh!

romani commented Dec 24, 2019

Uh oh!

shashvat-kedia commented Dec 24, 2019

Uh oh!

romani left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

romani commented Dec 24, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

shashvat-kedia commented Dec 25, 2019

Uh oh!

romani left a comment

Choose a reason for hiding this comment

Uh oh!

rnveach commented Dec 28, 2019

Uh oh!

romani commented Dec 28, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

shashvat-kedia commented Oct 27, 2019 •

edited by romani

Loading

shashvat-kedia commented Oct 27, 2019 •

edited

Loading

romani left a comment •

edited

Loading

romani left a comment •

edited

Loading

romani commented Dec 19, 2019 •

edited

Loading

romani commented Dec 24, 2019 •

edited

Loading