Issue #3951: SeparatorWrap: add support for method reference operator #4149

Luolc · 2017-04-03T12:02:16Z

Firstly, we need to confirm the check logic. As the issue mentioned: want '::' operator at end of previous line, but it is not correct according to google style: http://checkstyle.sourceforge.net/reports/google-java-style-20170228.html#s4.5.1-line-wrapping-where-to-break

When a line is broken at a non-assignment operator the break comes before the symbol.

This also applies to the following "operator-like" symbols:

the two colons of a method reference (::)

Notice that the break should come before ::.

Configurations for Google style is updated as well.

~~Diff report is on the way.~~

Diff report:
http://www.luolc.com/checkstyle-diff-report/issue3951/

codecov-io · 2017-04-03T12:19:33Z

Codecov Report

Merging #4149 into master will not change coverage.
The diff coverage is n/a.

@@          Coverage Diff           @@
##           master   #4149   +/-   ##
======================================
  Coverage     100%    100%           
======================================
  Files         283     283           
  Lines       14824   14824           
  Branches     3389    3389           
======================================
  Hits        14824   14824

Impacted Files	Coverage Δ
...om/puppycrawl/tools/checkstyle/api/TokenTypes.java	`100% <ø> (ø)`	⬆️
...eckstyle/checks/whitespace/SeparatorWrapCheck.java	`100% <ø> (ø)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update 460c185...104aa37. Read the comment docs.

MEZk · 2017-04-03T20:19:26Z

@Luolc
As METHOD_REF causes confusion, please add an example into xdoc which clearly shows where the violation should be and where it should not.

@Luolc
Please, update your checkstyle-tester local repo, we made some fixes.

@romani

Configurations for Google style is updated as well.

Should Google Style Guide be updated in the scope of this PR?

Luolc · 2017-04-03T21:33:31Z

@MEZk

please add an example into xdoc which clearly shows where the violation should be and where it should not.

Done. Please have a check whether the example is ok.

Please, update your checkstyle-tester local repo

Done. New diff report is generated. Posted at same location: http://www.luolc.com/checkstyle-diff-report/issue3951/

MEZk · 2017-04-03T21:36:49Z

@Luolc
This change is not in your local repo.

MEZk · 2017-04-03T21:38:20Z

src/xdocs/config_whitespace.xml

+          Example for method reference (good case and bad case):
+        </p>
+        <source>
+// violation, breaking after method reference is bad practice


Please, put the comment on the line where the violation is actually happens.

Luolc · 2017-04-03T21:45:48Z

@MEZk Oh... my fault. I only did rebase on my master branch. Sorry for that.
New report is on the way.

Luolc · 2017-04-03T23:07:20Z

@MEZk new report: http://www.luolc.com/checkstyle-diff-report/issue3951/
What does the warning means? Did I miss smth?

MEZk · 2017-04-04T20:53:02Z

@Luolc

What does the warning means? Did I miss smth?

It means that diff.groovy does not take into account any excludes specified in projects-to-test-on.properties. It runs the report generation against all source files of the target projects.
FYI: checkstyle/contribution#155

MEZk · 2017-04-04T20:58:34Z

src/xdocs/config_whitespace.xml

+          Example for method reference (good case and bad case):
+        </p>
+        <source>
+Arrays.sort(stringArray, String:: // violation, breaking after method reference is bad practice


It is not a general bad practice. User is able to decide whether he/she wants to configure the check to rise the violation if :: is on the next line after line break. Please remove breaking after method reference is bad practice.
Please, add an example of check's configuration for the given example as METHOD_REF is not in the set of default tokens.

romani · 2017-04-08T22:20:01Z

Firstly, we need to confirm the check logic. As the issue mentioned: want '::' operator at end of previous line, but it is not correct according to google style:

Styles are different from team to team, Check as ability to satisfy both of them by property option
http://checkstyle.sourceforge.net/property_types.html#wrapOp , http://checkstyle.sourceforge.net/config_whitespace.html#SeparatorWrap .

Should Google Style Guide be updated in the scope of this PR?

I did not find issue, looks like we miss it. @Luolc , please as you started to change ITs please do this in separate commit of this PR, please update google_checks.xml too.

romani

@Luolc , please split this commit into two:

Check udpate + checkstyle_checks.xml
google style related udpates (ITs, config)

Unfortunately empty reports mean no regression, but it does not mean that Check works as expected.
additionally , please generate reports on projects that have "::" in mode eol to see violations, lets validate violations.

…e operator

…tion of SeparatorWrap

Luolc · 2017-04-09T12:29:09Z

@romani

please split this commit into two

Done.

please generate reports on projects that have "::" in mode eol to see violations

Done. New report with EOL option: http://www.luolc.com/checkstyle-diff-report/issue3951/index.html
Still no regression.

One of Travis-CI's job failed. Would you please restart it? https://travis-ci.org/checkstyle/checkstyle/jobs/220246004

Error log of the failed job:

[ERROR] Failed to execute goal org.apache.maven.plugins:maven-surefire-plugin:2.19.1:test (default-test) on project checkstyle: Execution default-test of goal org.apache.maven.plugins:maven-surefire-plugin:2.19.1:test failed: The forked VM terminated without properly saying goodbye. VM crash or System.exit called?

rnveach · 2017-04-09T12:49:36Z

One of Travis-CI's job failed.

restarted

romani · 2017-04-09T13:37:12Z

Still no regression.

that is weird that with both options values there is no regression it could mean only that "::" is not used in this projects or there is not wraps ..... we need to see some violations on real code.

Please run Check with both values against https://github.com/javaslang/javaslang , if you can find any other project that have wrapping of "::" it will be awesome.

rnveach · 2017-04-09T14:51:53Z

@romani @Luolc
I ran report to show projects that are using :: and where if you wish to verify why any aren't causing differences.
http://rveach.no-ip.org/checkstyle/regression/reports/17/

Luolc · 2017-04-09T17:48:42Z

@romani
http://www.luolc.com/checkstyle-diff-report/issue3951-nl/
http://www.luolc.com/checkstyle-diff-report/issue3951-eol/
Besides javaslang, I also tried groovy and groovy-eclipse, the last two are the projects which @eric-milles are working on, the issue opener.

Unfortunately there are still no regression. :(

@rnveach I see wrapping of :: in checkstyle's code, but they are in the input files. Does diff report generator process them?

rnveach · 2017-04-09T18:26:35Z

Does diff report generator process them?

@Luolc Yes, this is why we added the red warning label to the diff reports to remind us we process all files in the project.

http://www.luolc.com/checkstyle-diff-report/issue3951-nl/groovy/index.html

@Luolc How did you run regression with tokens METHOD_REF?
It is a new token and not in base/master. I got the following exception when trying to run regression on it.

Caused by: com.puppycrawl.tools.checkstyle.api.CheckstyleException: Token "METHOD_REF" was not found in Acceptable tokens list in check com.puppycrawl.tools.checkstyle.checks.whitespace.SeparatorWrapCheck

Please show us your full command line you used.
Since this is a brand new token, you can only run launch.groovy on the change, not diff.groovy.

Luolc · 2017-04-09T18:48:49Z

@rnveach I found the error at first. To generate diff report, on my local master branch I add METHOD_REF to Acceptable tokens set (without changing other functional codes in the check).

I am not using my working PC currently. The diff report generation script is on it. I will update the command I used once I get my PC tomorrow.

Luolc · 2017-04-12T16:30:24Z

@rnveach @romani
The way I generated diff reports is not reasonable at all...

Since it is a new token I could only use launch.groovy.

http://www.luolc.com/checkstyle-diff-report/reports/issue3951/nl/
http://www.luolc.com/checkstyle-diff-report/reports/issue3951/eol/

These are the reports, we finally find :: with wrapping, but only in checkstyle, our project itself. And it is only in input files.

BTW, how could I generate reports with root page like http://rveach.no-ip.org/checkstyle/regression/reports/17/ ?
I run groovy launch.groovy -l projects-to-test-on.properties -c my_check.xml but only get separate folders without a root index.html. The pages above is written manually.

rnveach · 2017-04-12T17:39:51Z

BTW, how could I generate reports with root page like http://rveach.no-ip.org/checkstyle/regression/reports/17/ ?

@Luolc My system is my own creation. It was made before the groovy scripts which try to copy most of my functionality with added support of non-Linux OSes.
Part of it is located at https://github.com/checkstyle/contribution/blob/master/checkstyle-tester/launch_diff.sh but it isn't really recommended for official regression and it won't help you do the one sided regression mentioned above. I have another script which handles that which is not online. I can do various things in my system which groovy doesn't support yet.

I will make a new issue to have groovy do regression with no base for new checks/tokens.

romani · 2017-04-22T18:18:37Z

Firstly, we need to confirm the check logic. As the issue mentioned: want '::' operator at end of previous line, but it is not correct according to google style

In issue I did not found any, reference to google style, so issue was general and correct (violation was expected as config is required this)
But thanks a lot for update of Google config and its ITs - we missed this. Even http://checkstyle.sourceforge.net/google_style.html shows full coverage of for this point of style.

I am ok merge.
I am fine with reports. We tried enough to find regression. If some problem appear they will not be very obvious , so community will not blame us for simple regression.

@rnveach , please do final review and merge.

Luolc force-pushed the issue3951 branch from 337b310 to 24f994b Compare April 3, 2017 21:27

MEZk suggested changes Apr 3, 2017

View reviewed changes

Luolc force-pushed the issue3951 branch from 24f994b to d69b512 Compare April 3, 2017 21:47

MEZk suggested changes Apr 4, 2017

View reviewed changes

Luolc force-pushed the issue3951 branch from d69b512 to 8814189 Compare April 6, 2017 08:42

romani requested changes Apr 8, 2017

View reviewed changes

Luolc added 2 commits April 9, 2017 19:59

Issue checkstyle#3951: SeparatorWrap: add support for method referenc…

10ba15c

…e operator

Issue checkstyle#3951: update related config and ITs of METHOD_REF op…

104aa37

…tion of SeparatorWrap

Luolc force-pushed the issue3951 branch from 8814189 to 104aa37 Compare April 9, 2017 12:03

rnveach mentioned this pull request Apr 12, 2017

checkstyle-tester: support regression on new checks/tokens checkstyle/contribution#213

Closed

rnveach requested a review from romani April 19, 2017 10:48

romani approved these changes Apr 22, 2017

View reviewed changes

rnveach approved these changes Apr 26, 2017

View reviewed changes

rnveach merged commit fc2411a into checkstyle:master Apr 26, 2017

Luolc deleted the issue3951 branch April 27, 2017 04:21

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Issue #3951: SeparatorWrap: add support for method reference operator #4149

Issue #3951: SeparatorWrap: add support for method reference operator #4149

Luolc commented Apr 3, 2017 •

edited

Loading

codecov-io commented Apr 3, 2017 •

edited

Loading

MEZk commented Apr 3, 2017 •

edited

Loading

Luolc commented Apr 3, 2017

MEZk commented Apr 3, 2017

MEZk Apr 3, 2017

Luolc Apr 3, 2017

Luolc commented Apr 3, 2017

Luolc commented Apr 3, 2017

MEZk commented Apr 4, 2017 •

edited

Loading

MEZk Apr 4, 2017 •

edited

Loading

Luolc Apr 6, 2017

romani commented Apr 8, 2017 •

edited

Loading

romani left a comment •

edited

Loading

Luolc commented Apr 9, 2017

rnveach commented Apr 9, 2017

romani commented Apr 9, 2017

rnveach commented Apr 9, 2017

Luolc commented Apr 9, 2017

rnveach commented Apr 9, 2017 •

edited

Loading

Luolc commented Apr 9, 2017 •

edited

Loading

Luolc commented Apr 12, 2017

rnveach commented Apr 12, 2017 •

edited

Loading

romani commented Apr 22, 2017

Issue #3951: SeparatorWrap: add support for method reference operator #4149

Issue #3951: SeparatorWrap: add support for method reference operator #4149

Conversation

Luolc commented Apr 3, 2017 • edited Loading

codecov-io commented Apr 3, 2017 • edited Loading

Codecov Report

MEZk commented Apr 3, 2017 • edited Loading

Luolc commented Apr 3, 2017

MEZk commented Apr 3, 2017

MEZk Apr 3, 2017

Choose a reason for hiding this comment

Luolc Apr 3, 2017

Choose a reason for hiding this comment

Luolc commented Apr 3, 2017

Luolc commented Apr 3, 2017

MEZk commented Apr 4, 2017 • edited Loading

MEZk Apr 4, 2017 • edited Loading

Choose a reason for hiding this comment

Luolc Apr 6, 2017

Choose a reason for hiding this comment

romani commented Apr 8, 2017 • edited Loading

romani left a comment • edited Loading

Choose a reason for hiding this comment

Luolc commented Apr 9, 2017

rnveach commented Apr 9, 2017

romani commented Apr 9, 2017

rnveach commented Apr 9, 2017

Luolc commented Apr 9, 2017

rnveach commented Apr 9, 2017 • edited Loading

Luolc commented Apr 9, 2017 • edited Loading

Luolc commented Apr 12, 2017

rnveach commented Apr 12, 2017 • edited Loading

romani commented Apr 22, 2017

Luolc commented Apr 3, 2017 •

edited

Loading

codecov-io commented Apr 3, 2017 •

edited

Loading

MEZk commented Apr 3, 2017 •

edited

Loading

MEZk commented Apr 4, 2017 •

edited

Loading

MEZk Apr 4, 2017 •

edited

Loading

romani commented Apr 8, 2017 •

edited

Loading

romani left a comment •

edited

Loading

rnveach commented Apr 9, 2017 •

edited

Loading

Luolc commented Apr 9, 2017 •

edited

Loading

rnveach commented Apr 12, 2017 •

edited

Loading