Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Change LineLength Check parent from TreeWalker to Checker #2116

Closed
rnveach opened this issue Sep 4, 2015 · 18 comments

Comments

@rnveach
Copy link
Member

commented Sep 4, 2015

Why does LineLengthCheck need the java parser to work?
It could be be based on AbstractFileSetCheck and allow any file type to use this check, like property and xml files.

It returns nothing for acceptable/required/default TokenTypes.
The first thing it does is get all the lines of the file, which "processFiltered" will pass it.
It never visits anything or use any of the ASTs.
Only downside is that it would have to move out of the TreeWalker module in the configs.

ATTENTION: Migration Notes

Move LineLengthCheck outside of TreeWalker and place it directly in Checker.

By default, the changes to the check will scan all files not just Java files. If you want to restore the original behavior, using the following configuration with the fileExtensions property.

<module name="LineLengthCheck">
  <property name="fileExtensions" value="java" />
</module>

If there was some suppression in code by SuppressWithNearbyCommentFilter with influenceFormat ... unfortunately there is no similar filter of Checker, the only filter in Checker:

    <module name="SuppressWithPlainTextCommentFilter">
        <property name="offCommentFormat" value="CHECKSTYLE OFF\: ([\w\|]+)"/>
        <property name="onCommentFormat" value="CHECKSTYLE ON\: ([\w\|]+)"/>
        <property name="checkFormat" value="$1"/>
    </module>

see example of migration: checkstyle/equalsverifier@bce5a00

@romani

This comment has been minimized.

Copy link
Member

commented Sep 4, 2015

the reason my be pure historical, but as you make check as Check (not as FileSetCheck) you skip comments and empty lines (full of spaces and tabs) and you validate only lines of code where code is present.

We can not easily change a parent module , as we will brake all configurations in the world that have such Check. If you do not like such Check, we has to keep it, and create new Check.

For such questions please use our mail-lists http://checkstyle.sourceforge.net/mail-lists.html

@romani

This comment has been minimized.

Copy link
Member

commented May 17, 2018

you skip comments

this is probably valuable part of Check and some users might reuse it to bypass javadoc comments where URL always long.
If we endup in decision to convert Check we need to provide users ready to use updates to configuration to keep ignoring comments in addition to "package and imports".

@rnveach

This comment has been minimized.

Copy link
Member Author

commented May 17, 2018

as you make check as Check (not as FileSetCheck) you skip comments and empty lines (full of spaces and tabs) and you validate only lines of code where code is present.

This check does not do this. We are looking at all lines and never even touch AST to say 'skip line if not in AST'.
See https://github.com/checkstyle/checkstyle/blob/master/src/main/java/com/puppycrawl/tools/checkstyle/checks/sizes/LineLengthCheck.java#L113

Only update to configuration should be to move module outside TreeWalker.

@romani

This comment has been minimized.

Copy link
Member

commented May 17, 2018

This Check is comments unaware(it did not override required method), ones we convert it to FileSetCheck it will process each line of any file (java files with comments). So it will be change in config and change in behavior, we should do this with caution (to avoid cases as we had in Filters recently, when we damaged bunch of use cases for users, see #4714).

@rnveach

This comment has been minimized.

Copy link
Member Author

commented May 17, 2018

This Check is comments unaware

Doesn't matter. It doesn't have a visit method, and in beginTree it calls getLines which gets all lines which has no restriction on comment or code.

change in behavior

No it wont for this check.

$ cat TestClass.java
public class TestClass {

// BAD

}

$ cat TestConfig.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"/>

    <module name="TreeWalker">
<module name="LineLengthCheck">
  <property name="max" value="-1" />
  <property name="ignorePattern" value="DONT HIT ANY"/>
</module>
    </module>
</module>

$ java -jar checkstyle-8.10-all.jar -c TestConfig.xml TestClass.java
Starting audit...
[ERROR] TestClass.java:1: Line is longer than -1 characters (found 24). [LineLength]
[ERROR] TestClass.java:2: Line is longer than -1 characters (found 0). [LineLength]
[ERROR] TestClass.java:3: Line is longer than -1 characters (found 6). [LineLength]
[ERROR] TestClass.java:4: Line is longer than -1 characters (found 0). [LineLength]
[ERROR] TestClass.java:5: Line is longer than -1 characters (found 1). [LineLength]
Audit done.
Checkstyle ends with 5 errors.

You can already hit comments and blank lines. There will be no change in behavior in switch.

@romani

This comment has been minimized.

Copy link
Member

commented May 17, 2018

Doesn't matter. It doesn't have a visit method, and in beginTree it calls getLines

good point, so it is ok to convert Check to be FileSet.

@rnveach

This comment has been minimized.

Copy link
Member Author

commented Feb 20, 2019

One issue with making this change is the check uses tabWidth and it is only set by TreeWalker right now.
I don't see a reason why only TreeWalker gets access to the property and maybe consider breaking compatibility again and moving this property to Checker so it can be given to all checks that need it.

@romani

This comment has been minimized.

Copy link
Member

commented Feb 20, 2019

Agree. FileSet Checks should have tab size definition, but we can do this in certain Checks, rather than in Checker, as not all Checks should care about this.

@rnveach

This comment has been minimized.

Copy link
Member Author

commented Feb 20, 2019

The issue with putting it in a specific check only is that users will have to duplicate tabWidth to every check that needs it. Also, we will need it just for printing the violation correctly like we do in AbstractCheck which is why we can't do this same thing in another issue (#4111). Speaking of which AbstractFileSetCheck doesn't use tabWidth in the violation like AbstractCheck does, so the column numbers for violations is incorrect.

Example:

$ cat TestClass.java
public class TestClass {
    void method() {
//has no tabs
	//has tab
		//has 2 tabs
    }
}

$ cat TestConfig.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"/>

<module name="RegexpMultiline">
  <property name="format"
    value="//has"/>
</module>
</module>

$ java -jar checkstyle-8.17-all.jar -c TestConfig.xml TestClass.java
Starting audit...
[ERROR] TestClass.java:3: Line matches the illegal pattern '//has'. [RegexpMultiline]
[ERROR] TestClass.java:4: Line matches the illegal pattern '//has'. [RegexpMultiline]
[ERROR] TestClass.java:5: Line matches the illegal pattern '//has'. [RegexpMultiline]
Audit done.
Checkstyle ends with 3 errors.

Columns increase by 1 instead of by 4 or 8.

FileSet's log: https://github.com/checkstyle/checkstyle/blob/master/src/main/java/com/puppycrawl/tools/checkstyle/api/AbstractFileSetCheck.java#L155-L157
AbstractCheck's log: https://github.com/checkstyle/checkstyle/blob/master/src/main/java/com/puppycrawl/tools/checkstyle/api/AbstractCheck.java#L248-L256

@romani

This comment has been minimized.

Copy link
Member

commented Feb 21, 2019

Good point.
Ok to make property on Checker.

rnveach added a commit to rnveach/checkstyle that referenced this issue Apr 6, 2019
rnveach added a commit to rnveach/checkstyle that referenced this issue Apr 6, 2019
rnveach added a commit to rnveach/checkstyle that referenced this issue Aug 9, 2019
romani added a commit to rnveach/checkstyle that referenced this issue Aug 11, 2019
romani added a commit to rnveach/checkstyle that referenced this issue Aug 11, 2019
romani added a commit to rnveach/checkstyle that referenced this issue Aug 11, 2019
romani added a commit to rnveach/checkstyle that referenced this issue Aug 13, 2019
rnveach added a commit to rnveach/checkstyle that referenced this issue Aug 14, 2019
rnveach added a commit to rnveach/checkstyle that referenced this issue Aug 14, 2019
rnveach added a commit to rnveach/checkstyle that referenced this issue Aug 14, 2019
rnveach added a commit to rnveach/checkstyle that referenced this issue Aug 14, 2019
rnveach added a commit to rnveach/checkstyle that referenced this issue Aug 15, 2019
@romani

This comment has been minimized.

Copy link
Member

commented Aug 25, 2019

reopened, as wercker CI is failed:

[DEBUG] Configuring mojo org.apache.maven.plugins:maven-checkstyle-plugin:3.0.0:check from plugin realm ClassRealm[plugin>org.apache.maven.plugins:maven-checkstyle-plugin:3.0.0, parent: sun.misc.Launcher$AppClassLoader@33909752]
[DEBUG] Configuring mojo 'org.apache.maven.plugins:maven-checkstyle-plugin:3.0.0:check' with basic configurator -->
[DEBUG]   (f) cacheFile = /home/rivanov/java/github/romani/checkstyle/.ci-temp/xwiki-commons/target/checkstyle-cachefile
[DEBUG]   (f) configLocation = sun_checks.xml
[DEBUG]   (f) consoleOutput = true
[DEBUG]   (f) encoding = UTF-8
[DEBUG]   (f) failOnViolation = true
[DEBUG]   (f) failsOnError = false
[DEBUG]   (f) headerLocation = LICENSE.txt
[DEBUG]   (f) includeResources = true
[DEBUG]   (f) includeTestResources = true
[DEBUG]   (f) includeTestSourceDirectory = false
[DEBUG]   (f) includes = **\/*.java
[DEBUG]   (f) logViolationsToConsole = true
[DEBUG]   (f) maxAllowedViolations = 0
[DEBUG]   (f) omitIgnoredModules = false
[DEBUG]   (f) outputFile = /home/rivanov/java/github/romani/checkstyle/.ci-temp/xwiki-commons/target/checkstyle-result.xml
[DEBUG]   (f) outputFileFormat = xml

...
[ERROR] Failed to execute goal org.apache.maven.plugins:maven-checkstyle-plugin:3.0.0:check
 (default) on project xwiki-commons: Failed during checkstyle configuration: cannot initialize
 module TreeWalker - TreeWalker is not allowed as a parent of LineLength 
Please review 'Parent Module' section for this Check in web documentation if 
Check is standard. -> [Help 1]
org.apache.maven.lifecycle.LifecycleExecutionException: Failed to execute goal
 org.apache.maven.plugins:maven-checkstyle-plugin:3.0.0:check (default) on 
project xwiki-commons: Failed during checkstyle configuration

@rnveach , please update https://github.com/checkstyle/checkstyle/blob/master/src/main/resources/sun_checks.xml#L120 ... we do need test on this.

@romani romani reopened this Aug 25, 2019

@romani

This comment has been minimized.

Copy link
Member

commented Aug 25, 2019

We need to make execution similar to https://github.com/checkstyle/checkstyle/blob/master/.travis.yml#L146 just on sun_checks.xml

@rnveach

This comment has been minimized.

Copy link
Member Author

commented Aug 26, 2019

Which item at https://checkstyle.sourceforge.io/sun_style.html does the check apply to that it would go under it for the tests? Or you just want the regression only?

@romani

This comment has been minimized.

Copy link
Member

commented Aug 26, 2019

regression only for now, as we need to fix this ASAP.

rnveach added a commit to rnveach/checkstyle that referenced this issue Aug 26, 2019
rnveach added a commit to rnveach/checkstyle that referenced this issue Aug 26, 2019
romani added a commit to rnveach/checkstyle that referenced this issue Aug 26, 2019
romani added a commit that referenced this issue Aug 26, 2019
romani added a commit to romani/checkstyle that referenced this issue Aug 26, 2019
romani added a commit to romani/checkstyle that referenced this issue Aug 26, 2019

@romani romani changed the title Change LineLengthCheck to extend AbstractFileSetCheck Change LineLength Check parent from TreeWalker to Checker Aug 26, 2019

@romani

This comment has been minimized.

Copy link
Member

commented Aug 26, 2019

@rnveach , @pbludov , @strkkk , huge leak of CI damage happened because we use skip build options if commit contains only changes in other CIs configs ... main PR had few commits with changes to Travis/Shippable so wercker decided to skip validation at all build is skipped ... (see details). Please be careful.

We need to update logic to make such judgement only if there is single commit ... I not not sure for now how it is possible reliably do for all CIs.

@rnveach

This comment has been minimized.

Copy link
Member Author

commented Aug 26, 2019

We need to update logic to make such judgement only if there is single commit

This should be done in another, new, issue so we are sure we are tracking it. If you already know the details of the failure please create it and I will try to find some time tonight.

Right now master is green. So has CI been fixed and this issue can be closed?

@romani

This comment has been minimized.

Copy link
Member

commented Aug 26, 2019

Issue can be closed, I merged all fixes to CI.

@romani romani closed this Aug 26, 2019

rculbertson added a commit to rculbertson/spotify-checkstyle-config that referenced this issue Sep 4, 2019
Fix for breaking change in checkstyle 8.24
Checkstyle 8.24 has a breaking change which requires that the LineLength check be
declared outside the TreeWalker module. Here are links to the release notes and
the github issue describing this:

https://checkstyle.sourceforge.io/releasenotes.html#Release_8.24
checkstyle/checkstyle#2116

Note: this change will not work with checkstyle versions < 8.24
ngeor added a commit to ngeor/checkstyle-rules that referenced this issue Sep 7, 2019
Moving LineLength outside TreeWalker due to backwards compatibility i…
…ssue with checkstyle 8.24 ( checkstyle/checkstyle#2116 ).

Adding tests for the rules.
ngeor added a commit to ngeor/checkstyle-rules that referenced this issue Sep 7, 2019
Upgrade to checkstyle 8.24 (#3)
* Using Java 11. Corrected DTD.
* Upgraded plugins
* Added Maven Enforcer plugin
* Moving LineLength outside TreeWalker due to backwards compatibility issue with checkstyle 8.24 ( checkstyle/checkstyle#2116 ).
* Adding test support for the rules.
* Removed deploy script. Build script able to handle deployments.
* Updated documentation.
* Enabled rules OneTopLevelClass and OuterTypeFilename.
* Disabled rule TodoComment.
* Bump version
romani added a commit that referenced this issue Sep 15, 2019
romani added a commit that referenced this issue Sep 15, 2019
rnveach added a commit that referenced this issue Sep 15, 2019
romani added a commit that referenced this issue Sep 15, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
2 participants
You can’t perform that action at this time.