keep a map of Check name and it package in source to avoid brute force load by PackageObjectFactory from all packages #3184

Closed
romani opened this Issue May 17, 2016 · 2 comments

Comments

Projects
None yet
2 participants
@romani
Member

romani commented May 17, 2016

base on discussion at #2918.

Log events that make user worry that smth bad is happening:

[DEBUG] [com.puppycrawl.tools.checkstyle.PackageObjectFactory] Keep looking, ignoring exception
com.puppycrawl.tools.checkstyle.api.CheckstyleException: Unable to find class for com.puppycrawl.tools.checkstyle.checks.regexp.ParenPadCheck

PackageObjectFactory try to create Check from all possible packages. We need to make internal map of Check and its package for all standard Checks to load them directly without any guess.
This map content should be validated by UTs.

Verify in CLI that output is empty from Exceptions with FINEST logging level.

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Dec 2, 2016

Member

@Vladlis ,

Lets address this issue in few steps.

First:

  1. make hardcode map with all Check names that in have in checkstyle.
  2. Make UT that validate that map is in sync with actual code.
  3. Update code to do not log exception in debug mode - it confuse users more than bring benefit. Example: checkstyle/sonar-checkstyle#26
  4. We already define checkstyle_packages.xml file that describe in what packages we should search Checks defined by simpleName.
    It is completely ok to put in configuration Check that is defined by Fully Qualified Name to avoid brute force loading in all packages that it knows - there should be no brute force.
  5. Do merge and release it.

Second: #3607

Member

romani commented Dec 2, 2016

@Vladlis ,

Lets address this issue in few steps.

First:

  1. make hardcode map with all Check names that in have in checkstyle.
  2. Make UT that validate that map is in sync with actual code.
  3. Update code to do not log exception in debug mode - it confuse users more than bring benefit. Example: checkstyle/sonar-checkstyle#26
  4. We already define checkstyle_packages.xml file that describe in what packages we should search Checks defined by simpleName.
    It is completely ok to put in configuration Check that is defined by Fully Qualified Name to avoid brute force loading in all packages that it knows - there should be no brute force.
  5. Do merge and release it.

Second: #3607

romani added a commit that referenced this issue Dec 20, 2016

@romani romani added the bug label Dec 20, 2016

@romani romani added this to the 7.4 milestone Dec 20, 2016

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Dec 20, 2016

Member

fix is merged.
new output is like this:

$ java -jar checkstyle-7.4-SNAPSHOT-all.jar -d -c checkstyle.xml Test.java
Dec 20, 2016 1:45:47 PM com.puppycrawl.tools.checkstyle.Main runCli
FINE: Checkstyle debug logging enabled
Dec 20, 2016 1:45:47 PM com.puppycrawl.tools.checkstyle.Main runCli
FINE: Running Checkstyle with version: 7.4-SNAPSHOT
Starting audit...
[ERROR] .....Test.java:9: 'case' construct must use '{}'s. [NeedBraces]
[ERROR] .....Test.java:12: 'case' construct must use '{}'s. [NeedBraces]
[ERROR] .....Test.java:15: 'default' construct must use '{}'s. [NeedBraces]
Audit done.
Checkstyle ends with 3 errors.

Member

romani commented Dec 20, 2016

fix is merged.
new output is like this:

$ java -jar checkstyle-7.4-SNAPSHOT-all.jar -d -c checkstyle.xml Test.java
Dec 20, 2016 1:45:47 PM com.puppycrawl.tools.checkstyle.Main runCli
FINE: Checkstyle debug logging enabled
Dec 20, 2016 1:45:47 PM com.puppycrawl.tools.checkstyle.Main runCli
FINE: Running Checkstyle with version: 7.4-SNAPSHOT
Starting audit...
[ERROR] .....Test.java:9: 'case' construct must use '{}'s. [NeedBraces]
[ERROR] .....Test.java:12: 'case' construct must use '{}'s. [NeedBraces]
[ERROR] .....Test.java:15: 'default' construct must use '{}'s. [NeedBraces]
Audit done.
Checkstyle ends with 3 errors.

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