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

PackageObjectFactory can't instantiate AuditListeners #4742

Closed
rnveach opened this Issue Jul 17, 2017 · 11 comments

Comments

Projects
None yet
2 participants
@rnveach
Member

rnveach commented Jul 17, 2017

I am trying to upgrade my project and custom jars from Checkstyle 7.5.1 to 8.0 and I am getting an error that one of my custom AuditListeners can't be instantiated by name alone even though it's package is listed in the checkstyle_packages.xml file.

The issue is the new code in PackageObjectFactory isn't looking for AuditListeners.
isCheckstyleModule is looking for everything else except AuditListeners.
It needs to be expanded to scan for them in 3rd party packages. ModuleReflectionUtils requires a isAuditListener method as part of isCheckstyleModule.

I wrote a custom listener and expanded what communications it receives to gather and print metrics on it's execution time and number of times areas were called to produce reports and examine slow down areas, like modules and javadoc. You have seen these reports before. FileSets and Checks cannot receive the detailed information that a listener can as it is the most global thing we have in Checkstyle.

This is my branch: https://github.com/rnveach/checkstyle/commits/more_audits

I also have a private listener which is basically an implementation of #3242 , where on auditFinished it examines the list of defined suppressions and prints violations on any that were not used.
I have to use a listener as it is the only customizable area that is called at the end once all other modules, filters, and such are done.

Listener can be defined in the configuration.

@rnveach

This comment has been minimized.

Show comment
Hide comment
@rnveach

rnveach Aug 5, 2017

Member

@romani Upon adding and recognizing listeners, I am getting the following test errors:

[ERROR] Failures: 
[ERROR]   PackageObjectFactoryTest.testNameToFullModuleNameMap:254 Invalid canonical name
[ERROR]   AllChecksTest.testAllCheckstyleModulesHaveXdocDocumentation:422->lambda$testAllCheckstyleModulesHaveXdocDocumentation$3:426 Module DefaultLogger does not have xdoc documentation.
[ERROR]   AllChecksTest.testAllCheckstyleModulesInCheckstyleConfig:435 checkstyle_checks.xml is missing module: DefaultLogger
[ERROR]   AllChecksTest.testAllModulesAreReferencedInConfigFile:325->lambda$testAllModulesAreReferencedInConfigFile$1:328 DefaultLogger is not referenced in checkstyle_checks.xml
[ERROR] Errors: 
[ERROR]   AllChecksTest.testAllModulesWithDefaultConfiguration:257->AbstractModuleTestSupport.createChecker:94->AbstractModuleTestSupport.createChecker:119 » Checkstyle

So I have the following questions/confirmations:

  1. Should our listeners be defined in PackageObjectFactory so that they can be referenced by short name? Should we even allow this as they are more for CLI use?
  2. Should our listeners have their own xdocs page?
  3. I assume we shouldn't register our own listeners inside checkstyle_checks.xml?
Member

rnveach commented Aug 5, 2017

@romani Upon adding and recognizing listeners, I am getting the following test errors:

[ERROR] Failures: 
[ERROR]   PackageObjectFactoryTest.testNameToFullModuleNameMap:254 Invalid canonical name
[ERROR]   AllChecksTest.testAllCheckstyleModulesHaveXdocDocumentation:422->lambda$testAllCheckstyleModulesHaveXdocDocumentation$3:426 Module DefaultLogger does not have xdoc documentation.
[ERROR]   AllChecksTest.testAllCheckstyleModulesInCheckstyleConfig:435 checkstyle_checks.xml is missing module: DefaultLogger
[ERROR]   AllChecksTest.testAllModulesAreReferencedInConfigFile:325->lambda$testAllModulesAreReferencedInConfigFile$1:328 DefaultLogger is not referenced in checkstyle_checks.xml
[ERROR] Errors: 
[ERROR]   AllChecksTest.testAllModulesWithDefaultConfiguration:257->AbstractModuleTestSupport.createChecker:94->AbstractModuleTestSupport.createChecker:119 » Checkstyle

So I have the following questions/confirmations:

  1. Should our listeners be defined in PackageObjectFactory so that they can be referenced by short name? Should we even allow this as they are more for CLI use?
  2. Should our listeners have their own xdocs page?
  3. I assume we shouldn't register our own listeners inside checkstyle_checks.xml?
@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Aug 6, 2017

Member

Should our listeners be defined in PackageObjectFactory so that they can be referenced by short name?

short names are only for human in config file, there are too much names.
All other places should use full names.

Should our listeners have their own xdocs page?

hmm, they could, but users do not need listeners and do not need customization of them.
Listeners are required for engineers who want to do smth custom, they will look at code for latest state of class.
So xdoc for listeners is probably not required.

I assume we shouldn't register our own listeners inside checkstyle_checks.xml?

I do not know a use case when it make sense. For now it is not required. Output format is a business of caller (plugin, CLI, .....)

Member

romani commented Aug 6, 2017

Should our listeners be defined in PackageObjectFactory so that they can be referenced by short name?

short names are only for human in config file, there are too much names.
All other places should use full names.

Should our listeners have their own xdocs page?

hmm, they could, but users do not need listeners and do not need customization of them.
Listeners are required for engineers who want to do smth custom, they will look at code for latest state of class.
So xdoc for listeners is probably not required.

I assume we shouldn't register our own listeners inside checkstyle_checks.xml?

I do not know a use case when it make sense. For now it is not required. Output format is a business of caller (plugin, CLI, .....)

@rnveach

This comment has been minimized.

Show comment
Hide comment
@rnveach

rnveach Aug 7, 2017

Member

I assume we shouldn't register our own listeners inside checkstyle_checks.xml?

For now it is not required.

We can't because these listeners don't have a default constructor, so they fail to instantiate. I missed this before.
We should probably add that as a requirement in ModuleReflectionUtils.

Member

rnveach commented Aug 7, 2017

I assume we shouldn't register our own listeners inside checkstyle_checks.xml?

For now it is not required.

We can't because these listeners don't have a default constructor, so they fail to instantiate. I missed this before.
We should probably add that as a requirement in ModuleReflectionUtils.

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Aug 7, 2017

Member

requires a isAuditListener method as part of isCheckstyleModule

Listener is not a Module ...... such update is not ok.... I removing "approved" label for now to make it clear what we are going to do.

Member

romani commented Aug 7, 2017

requires a isAuditListener method as part of isCheckstyleModule

Listener is not a Module ...... such update is not ok.... I removing "approved" label for now to make it clear what we are going to do.

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Aug 7, 2017

Member

https://github.com/checkstyle/checkstyle/blob/master/src/main/java/com/puppycrawl/tools/checkstyle/DefaultLogger.java#L44

public class DefaultLogger extends AutomaticBean implements AuditListener {

it might be used in config file only due to extends AutomaticBean .
This become a nuance , as always thought about config as way to define how validation should be done, but not way to define destination of output. That was a tasks of caller, CLI, plugin, application .... .
It is good question if we should continue to support this. I need to mediate about this concept for some time to make a decision is good or bad for checkstyle.

Member

romani commented Aug 7, 2017

https://github.com/checkstyle/checkstyle/blob/master/src/main/java/com/puppycrawl/tools/checkstyle/DefaultLogger.java#L44

public class DefaultLogger extends AutomaticBean implements AuditListener {

it might be used in config file only due to extends AutomaticBean .
This become a nuance , as always thought about config as way to define how validation should be done, but not way to define destination of output. That was a tasks of caller, CLI, plugin, application .... .
It is good question if we should continue to support this. I need to mediate about this concept for some time to make a decision is good or bad for checkstyle.

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Aug 7, 2017

Member

if we allow AuditListeners to be in config, so it mean that user is allowed to define output by config.
It will be done on addition to what it defined by caller code(CLI, plugin, application)

https://github.com/checkstyle/checkstyle/blob/master/src/main/java/com/puppycrawl/tools/checkstyle/Main.java#L499

           rootModule.setModuleClassLoader(moduleClassLoader);
           rootModule.configure(config);
           rootModule.addListener(listener);

If we allow AuditListeners to be in config, we should allow NO OUTPUT mode in our CLI to let user control all by config.

Am I make sense ?

Member

romani commented Aug 7, 2017

if we allow AuditListeners to be in config, so it mean that user is allowed to define output by config.
It will be done on addition to what it defined by caller code(CLI, plugin, application)

https://github.com/checkstyle/checkstyle/blob/master/src/main/java/com/puppycrawl/tools/checkstyle/Main.java#L499

           rootModule.setModuleClassLoader(moduleClassLoader);
           rootModule.configure(config);
           rootModule.addListener(listener);

If we allow AuditListeners to be in config, we should allow NO OUTPUT mode in our CLI to let user control all by config.

Am I make sense ?

@rnveach

This comment has been minimized.

Show comment
Hide comment
@rnveach

rnveach Aug 7, 2017

Member

but not way to define destination of output

Just because it is a listener, doesn't mean it will only be used for output. If it was used for output alone, then adding multiple probably wouldn't make sense.
One example from http://checkstyle.sourceforge.net/writinglisteners.html:

Another user would like to filter error events.

Member

rnveach commented Aug 7, 2017

but not way to define destination of output

Just because it is a listener, doesn't mean it will only be used for output. If it was used for output alone, then adding multiple probably wouldn't make sense.
One example from http://checkstyle.sourceforge.net/writinglisteners.html:

Another user would like to filter error events.

@rnveach

This comment has been minimized.

Show comment
Hide comment
@rnveach

rnveach Aug 7, 2017

Member

If we allow AuditListeners to be in config, we should allow NO OUTPUT mode in our CLI to let user control all by config.

This makes sense if listener wants to control output fully. Only one of my listeners does this, but it has no negative effects working with the normal output listeners.
I am fine with this.

Member

rnveach commented Aug 7, 2017

If we allow AuditListeners to be in config, we should allow NO OUTPUT mode in our CLI to let user control all by config.

This makes sense if listener wants to control output fully. Only one of my listeners does this, but it has no negative effects working with the normal output listeners.
I am fine with this.

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Aug 17, 2017

Member

changes to CLI and AuditListener review is moved to #4956 .

Member

romani commented Aug 17, 2017

changes to CLI and AuditListener review is moved to #4956 .

@rnveach

This comment has been minimized.

Show comment
Hide comment
@rnveach

rnveach Aug 17, 2017

Member

@romani

such update is not ok.... I removing "approved" label for now to make it clear what we are going to do.

So this is now approved again?

Member

rnveach commented Aug 17, 2017

@romani

such update is not ok.... I removing "approved" label for now to make it clear what we are going to do.

So this is now approved again?

@romani romani added the approved label Aug 18, 2017

@romani romani added this to the 8.2 milestone Aug 18, 2017

@romani romani added the bug label Aug 18, 2017

romani added a commit that referenced this issue Aug 18, 2017

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Aug 18, 2017

Member

fix is merged

Member

romani commented Aug 18, 2017

fix is merged

@romani romani closed this Aug 18, 2017

ArneLimburg pushed a commit to ArneLimburg/checkstyle that referenced this issue Aug 23, 2017

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