Issue #4735: make RegexpHeaderCheck detect regex having '\n\n' #4768

vivekrao1985 · 2017-07-19T19:26:53Z

romani · 2017-07-19T19:53:46Z

ALL CIs need to be green,
regression testing is required, please use diff.groovy from https://github.com/checkstyle/contribution/tree/master/checkstyle-tester, please read README for details on how to run and share report with us.

looks like no thing valuable we can add to documentation https://github.com/checkstyle/checkstyle/blob/master/src/xdocs/config_header.xml#L186 it is source of html web site, as it simple bug.

codecov-io · 2017-07-19T20:28:07Z

Codecov Report

Merging #4768 into master will not change coverage.
The diff coverage is 100%.

@@          Coverage Diff           @@
##           master   #4768   +/-   ##
======================================
  Coverage     100%    100%           
======================================
  Files         287     287           
  Lines       15482   15484    +2     
  Branches     3514    3515    +1     
======================================
+ Hits        15482   15484    +2

Impacted Files	Coverage Δ
.../checkstyle/checks/header/AbstractHeaderCheck.java	`100% <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 70761aa...66832a6. Read the comment docs.

vivekrao1985 · 2017-07-19T21:35:45Z

@romani - I get errors when running that regression script -

[WARNING] Error injecting: org.apache.maven.repository.internal.DefaultVersionResolver
com.google.inject.ProvisionException: Guice provision errors:

1) Error injecting: private org.eclipse.aether.spi.log.Logger org.eclipse.aether.internal.impl.DefaultOfflineController.logger
  while locating org.eclipse.aether.internal.impl.DefaultOfflineController
  while locating java.lang.Object annotated with *
  at org.eclipse.sisu.wire.LocatorWiring
  while locating org.eclipse.aether.impl.OfflineController

There's about 8 errors like this.

Command I ran -

groovy diff.groovy --localGitRepo /path/to//local/checkstyle --baseBranch master --patchBranch issue-4735 --config my_check.xml --listOfProjects projects-to-test-on.properties

my-check.xml -

<?xml version="1.0"?>
<!DOCTYPE module PUBLIC
          "-//Puppy Crawl//DTD Check Configuration 1.3//EN"
          "http://www.puppycrawl.com/dtds/configuration_1_3.dtd">


<module name = "Checker">
    <property name="charset" value="UTF-8"/>

    <!-- do not change severity to 'error', as that will hide errors caused by exceptions -->
    <property name="severity" value="warning"/>

    <!-- haltOnException is required for exception fixes and reporting of all exceptions -->
    <property name="haltOnException" value="false"/>

    <!-- BeforeExecutionFileFilters is required for sources of java9 -->
    <module name="BeforeExecutionExclusionFileFilter">
        <property name="fileNamePattern" value="module\-info\.java$" />
    </module>

    <module name="RegexpHeader">
        <property name="header" value="^package .*\n\n.*"/>
        <property name="fileExtensions" value="java"/>
    </module>

    <module name="TreeWalker">
         <!-- Example of sevntu.checkstyle Check usage -->
         <!-- <module name="NestedSwitchCheck"/> -->

         <!-- Example of checkstyle Check usage -->
    </module>
<!--
    <module name="SeverityMatchFilter">
        <property name="severity" value="warning"/>
        <property name="acceptOnMatch" value="false"/>
    </module>
-->
</module>

I did not change projects-to-test-on.properties.

vivekrao1985 · 2017-07-20T00:43:38Z

Turned out to be a version mismatch. You can find my test results here.

rnveach · 2017-07-20T02:25:36Z

Turned out to be a version mismatch.

Can you elaborate more? You mean a maven version mismatch or something else?

rnveach · 2017-07-20T02:26:43Z

You can find my test results here.

Please expand testing to all projects. A small test bed is usually not enough to find all the weird quirks and interesting cases.

vivekrao1985 · 2017-07-20T02:52:33Z

@rnveach - yes, it was a maven version mismatch. I was using version 3.2.1. I upgraded to 3.5 and it works now.

By all projects you mean uncomment all the projects in projects-to-test-on.properties?

rnveach · 2017-07-20T11:53:20Z

By all projects you mean uncomment all the projects in projects-to-test-on.properties?

Yes.

vivekrao1985 · 2017-07-20T16:09:02Z

@rnveach done.

romani

Diff reports are always better place to spot weird stuff
Here it is:
header=^package .\n\n.
Message=Line does not match expected header line of '^$'

It is completely unclear why non specified regexp is used.
We need to document this with example.

Please provide update to documentation file.

romani · 2017-07-27T16:00:42Z

@vivekrao1985 , please rebase to resovle conflicts and provide fix for documentation , we are very close to merge.

vivekrao1985 · 2017-07-31T19:35:03Z

Hi, Sorry, got a little busy. I will fix this sometime today.

vivekrao1985 · 2017-08-02T22:44:08Z

@romani I have rebased and provided documentation for this change.

vivekrao1985 · 2017-08-02T23:15:52Z

Looks like wercker build failed. How do I see the results? When I click on Details I get 401 - Unauthorized. The problem persists even after trying to login with github credentials.

rnveach · 2017-08-03T00:59:25Z

When I click on Details I get 401 - Unauthorized.

This is a issue with wercker. We have no control to give anonymous access.
Wercker failure is:

NoExceptiontest - Apache Struts
error: patch failed: checkstyle-tester/checks-nonjavadoc-error.xml:409

@timurt @romani This error is happening in master. It was passing before.

timurt · 2017-08-03T06:51:45Z

@rnveach

@timurt @romani This error is happening in master. It was passing before.

Project chekstyle-tester was updated recently a day ago, FileContentsHolder module was removed. We don't need patching checks-nonjavadoc-error.xml anymore. I will prepare separate PR to resolve problem.

romani · 2017-08-03T09:09:55Z

@vivekrao1985 , wercker fix is merged to master, please rebase on latest master to make all CIs happy.

romani

items to improve:

romani · 2017-08-03T09:20:35Z

src/xdocs/config_header.xml

@@ -225,7 +225,8 @@ line 14: ^ \*/
          four digit year. Line 5 is an example how to enforce revision
          control keywords in a file header. Lines 12-14 is a template for
          javadoc (line 13 is so complicated to remove conflict with and of
-          javadoc comment).
+          javadoc comment). Lines 7, 9 and 11 will be treated as '^$' and
+          will forcefully expect the line to be empty.


It will be very useful to users if you put this tiny nuance to header description the required header specified inline. Individual header lines must be separated by the strin..........

please also add that explanation to last example To configure the check to verify that each file starts with the header., or create new example. Users like examples more that boring manuals.

vivekrao1985 · 2017-08-03T11:00:49Z

Rebased and provided examples.

romani · 2017-08-03T14:31:07Z

@rnveach , please do final review

rnveach

Everything else looks fine.

rnveach · 2017-08-03T16:40:44Z

src/xdocs/config_header.xml

+        <source>
+package com.some.package;
+
+public class ThisWillPass { }


In your before example you had an empty javadoc. In this after example, there is no javadoc.
It is slightly misleading in that it makes it sound like you are forcing users to remove javadoc.

Either

restore javadoc in after example with empty line after package,

or remove it in before example so that package and public are on consecutive lines.

Makes sense, fixed.

…x having '\n\n'

vivekrao1985 force-pushed the issue-4735 branch from 95e1467 to 749af5d Compare July 19, 2017 20:59

rnveach mentioned this pull request Jul 20, 2017

Document maven version required for groovy checkstyle/contribution#255

Open

romani requested changes Jul 20, 2017

View reviewed changes

vivekrao1985 force-pushed the issue-4735 branch 4 times, most recently from 94494c0 to b7aa926 Compare August 2, 2017 22:40

vivekrao1985 force-pushed the issue-4735 branch from b7aa926 to 1c2deb5 Compare August 2, 2017 22:54

romani requested changes Aug 3, 2017

View reviewed changes

vivekrao1985 force-pushed the issue-4735 branch from 1c2deb5 to 8aedcb7 Compare August 3, 2017 10:56

romani approved these changes Aug 3, 2017

View reviewed changes

rnveach suggested changes Aug 3, 2017

View reviewed changes

vivekrao1985 force-pushed the issue-4735 branch from 8aedcb7 to 66832a6 Compare August 3, 2017 17:25

Issue checkstyle#4735: make RegexpHeaderCheck detect and enforce rege…

66832a6

…x having '\n\n'

rnveach approved these changes Aug 3, 2017

View reviewed changes

rnveach merged commit 93fad34 into checkstyle:master Aug 3, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Issue #4735: make RegexpHeaderCheck detect regex having '\n\n' #4768

Issue #4735: make RegexpHeaderCheck detect regex having '\n\n' #4768

vivekrao1985 commented Jul 19, 2017 •

edited

Loading

romani commented Jul 19, 2017 •

edited

Loading

codecov-io commented Jul 19, 2017 •

edited

Loading

vivekrao1985 commented Jul 19, 2017

vivekrao1985 commented Jul 20, 2017

rnveach commented Jul 20, 2017

rnveach commented Jul 20, 2017

vivekrao1985 commented Jul 20, 2017

rnveach commented Jul 20, 2017

vivekrao1985 commented Jul 20, 2017

romani left a comment

romani commented Jul 27, 2017

vivekrao1985 commented Jul 31, 2017

vivekrao1985 commented Aug 2, 2017

vivekrao1985 commented Aug 2, 2017

rnveach commented Aug 3, 2017

timurt commented Aug 3, 2017 •

edited

Loading

romani commented Aug 3, 2017 •

edited

Loading

romani left a comment

romani Aug 3, 2017

vivekrao1985 commented Aug 3, 2017

romani commented Aug 3, 2017

rnveach left a comment

rnveach Aug 3, 2017 •

edited

Loading

vivekrao1985 Aug 3, 2017

Issue #4735: make RegexpHeaderCheck detect regex having '\n\n' #4768

Issue #4735: make RegexpHeaderCheck detect regex having '\n\n' #4768

Conversation

vivekrao1985 commented Jul 19, 2017 • edited Loading

romani commented Jul 19, 2017 • edited Loading

codecov-io commented Jul 19, 2017 • edited Loading

Codecov Report

vivekrao1985 commented Jul 19, 2017

vivekrao1985 commented Jul 20, 2017

rnveach commented Jul 20, 2017

rnveach commented Jul 20, 2017

vivekrao1985 commented Jul 20, 2017

rnveach commented Jul 20, 2017

vivekrao1985 commented Jul 20, 2017

romani left a comment

Choose a reason for hiding this comment

romani commented Jul 27, 2017

vivekrao1985 commented Jul 31, 2017

vivekrao1985 commented Aug 2, 2017

vivekrao1985 commented Aug 2, 2017

rnveach commented Aug 3, 2017

timurt commented Aug 3, 2017 • edited Loading

romani commented Aug 3, 2017 • edited Loading

romani left a comment

Choose a reason for hiding this comment

romani Aug 3, 2017

Choose a reason for hiding this comment

vivekrao1985 commented Aug 3, 2017

romani commented Aug 3, 2017

rnveach left a comment

Choose a reason for hiding this comment

rnveach Aug 3, 2017 • edited Loading

Choose a reason for hiding this comment

vivekrao1985 Aug 3, 2017

Choose a reason for hiding this comment

vivekrao1985 commented Jul 19, 2017 •

edited

Loading

romani commented Jul 19, 2017 •

edited

Loading

codecov-io commented Jul 19, 2017 •

edited

Loading

timurt commented Aug 3, 2017 •

edited

Loading

romani commented Aug 3, 2017 •

edited

Loading

rnveach Aug 3, 2017 •

edited

Loading