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
Allow 3rd party Check providers to group modules under custom parent module #11644
Conversation
Old Idea: Our current module identification process is very strict. This PR is to allow that to be loosened so 3rd parties can create their own sub-modules that our current one doesn't support. To be sure to note, this is not changing
Suppose I had created a new walker With this new recognition, Reset is more for testing to show it works and not override the default in all areas. @romani You have any thoughts on this PR? |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let me find time to look at it closely
@romani ping |
a79c328
to
be9410b
Compare
b3a6354
to
b128d71
Compare
CI is passing here. |
@rnveach, Can we provide identification function to ctor of checkstyle/src/main/java/com/puppycrawl/tools/checkstyle/PackageObjectFactory.java Line 122 in a8f0657
Or some other place. |
Your limited in where this logic can go as PackageObjectFactory#createModule is called by the CLI (roots), checker (file sets), and treewalker (abstract checks) The only concern here is that basically createModule will return the first class it finds that seems valid. It won't search for another if the caller class doesn't accept the returning type. This should only really happen on clashing class names but defining the package will fix this. |
src/main/java/com/puppycrawl/tools/checkstyle/PackageObjectFactory.java
Outdated
Show resolved
Hide resolved
This comment was marked as outdated.
This comment was marked as outdated.
51b9eec
to
3bbcfd0
Compare
This comment was marked as outdated.
This comment was marked as outdated.
647f871
to
a5e57bc
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
item:
src/main/java/com/puppycrawl/tools/checkstyle/utils/ModuleReflectionUtil.java
Outdated
Show resolved
Hide resolved
Not understanding why https://codecov.io/gh/checkstyle/checkstyle/pull/11644
Edit: |
@rnveach , please update PR description to not mention old ideas (we can move them to comments to preserve ). Please use PR number in commit, as issue is like poroject umbrella, too much different updates. I think titile of PR is good, so users might notice it in release notes. |
Done.
Done.
I am thinking new feature, as we never really considered allowing these type of sub-modules. We may have had discussions and code changes implying this, but it was never out right said or probably documented. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We need to improve title of PR to be more easy to understand in release notes.
Maybe later on update PR description to put link to xml linter project as example.
Maybe good to update some doc page on how to do custom parent module.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Maybe good to update some doc page on how to do custom parent module.
Agreed, if we do all this work at allow Checkstyle to be more extensible, but do not have some documentation/ recommendation/ examples about how to do it, the value of resolving #11604 to users is probably low.
@rnveach can we add a healthy documentation update to tasks in #11604 ?
"healthy" is a strong word. Truthfully this PR is the first one that actually does something for 3rd parties. I can add documentation, but what are our thoughts regarding it? Are we requesting a blurb on https://checkstyle.org/writingchecks.html with something titled like "How to incorporate sub-checks"? |
I recommend to merge, let @rnveach use it in separate project, and then write doc. |
I do not know yet, hopefully experience from https://github.com/rnveach/checkstyle-extras will tell :)
@romani see #11644 (review), I would just like to have documentation update task added to list at #11604. I agree that doc update should wait until checkstyle-extras is closer to completion. |
@rnveach , please and sub task to big issue and let's move further |
Done. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice side effect of "killing" seven mutations too :)
Subset of Issue #11604
Our current module identification process is very strict. This PR is to allow that to be loosened so 3rd parties can create their own sub-modules that our current one doesn't support.
Suppose I had created a new walker
XmlWalker
which extendsAbstractFileSetCheck
and I wanted it to allow children modules likeAbstractXmlCheck
. Checkstyle will recognizeXmlWalker
because ofAbstractFileSetCheck
but it will not identify anything extendingAbstractXmlCheck
even if follows the absolute same makeup asAbstractCheck
. This is because we only recognize our abstracts (AbstractFileSetCheck
,AbstractCheck
, etc...) and nothing else that could be a child module. There is no common class for module children. Our packages.xml file will not help here, because again, we still only pull in things that follow our defined class hierarchy.With this change we will now be able to pick up these new children, as long as they follow basic rules defined by our
isCheckstyleModule
, and treat them like they are any other module we have for Checkstyle, as long as the parent module will accept them.isCheckstyleModule
defines things as: must extendAutomaticBean
, can't be abstract, must have a default constructor, and must not be an internal module to us.Example of project that has custom children modules: https://github.com/rnveach/checkstyle-extras