Module Component

[Luolc]

Taken from #2
We need to identify the type of file to restrict to only modules (for now). In the future, we also need to identify if the module has new/removed properties as they will need to be included in the regression.

[Luolc]

As pointed at #1 (review). The method was not a good implementation, for it just instantiate the class in the release version of checkstyle, and that is meaningless. I haven't think out a way to load the class from file and meanwhile avoid conflicts by now.

[Luolc]

For the branch version of checkstyle, we only have the java source code. If we want to use the ModuleReflectionUtils.isCheckstyleModule way, then we must install the branch version. So we have to do some initialization(compile and install the branch version) before we run the regression-tool. Even in this way we are not able to load checkstyle stable version and branch version in the meantime.
A more interesting question is how to deal with the change of classes like ModuleReflectionUtils, as they are used in the regression-tool project. It seems a infinite cycle, well, I don't know how to describe it in English.

From other point of view, I think we have some other way to avoid loading the branch version file by now. Currently, our goal is to judge whether a class is a checkstyle module. And we have the hardcode map in PackageObjectFactory. We could grab this info from the content of PackageObejctFactory. Maybe a better way is to split the map into a separate xml, then we could load the xml directly.

[Luolc]

According to https://github.com/checkstyle/checkstyle/blob/master/src/main/resources/com/puppycrawl/tools/checkstyle/configuration_1_3.dtd, we have module, property and message element. (I don't know what's the use of metadata).

1. Is it possible to generate config with message element? I haven't used that in regression by now, so I am not clear.
2. What is the use of default key of property element? I don't see it in the config of checkstyle repo.

<!ELEMENT property EMPTY>
<!ATTLIST property
    name NMTOKEN #REQUIRED
    value CDATA #REQUIRED
    default CDATA #IMPLIED
>

[rnveach]

I advise against asking free form questions in issue as it will cloud main discussion. This sounds like it would be better in gitter or google groups.

I don't know what's the use of metadata

See checkstyle/checkstyle#2726 .

Is it possible to generate config with message element?

Configuration can override module's message.

What is the use of default key of property element?

See https://github.com/checkstyle/checkstyle/blob/ce21086e087661553f3a774c38362327ee88996a/src/main/java/com/puppycrawl/tools/checkstyle/ConfigurationLoader.java#L255-L256 .
I have never used it before and assume it's main usage if property value is defined by external property and it isn't filled in at time of execution.

It seems a infinite cycle, well, I don't know how to describe it in English.

I describe it as the chicken and the egg scenario. https://en.wikipedia.org/wiki/Chicken_or_the_egg
The chicken lays eggs, but the chicken also hatches from an egg. So which came first?
Circular reference or cycle comes to mind. https://en.wikipedia.org/wiki/Circular_reference#In_computer_programming

[rnveach]

our goal is to judge whether a class is a checkstyle module

Our goal right now is if something is a module. Eventually, we will need more than modules. We will need to know properties, their types, and possibly their default values, tokens, messages, acceptable parent (TreeWalker or Checker), etc.

split the map into a separate xml, then we could load the xml directly.

We should avoid asking for changes in main project if we can. I am not against it, but it should be close to a last resort.

There has been talk about having some file in Checkstyle with all this information so 3rd party plugins could make use of it for their own purposes.
This sounds like the cleanest way but Checkstyle would need to enforce this is kept up to date even in PRs.

we must install the branch version ... before we run the regression-tool

Instead of installing, we would have to attach PR Checkstyle JAR to classpath at runtime.
Users would need to give us this Git PR location, branch name, as well as add this JAR.

We must also be sure we never need runtime information from multiple branches, like master.
Do we need to know if a property/message/token is new/removed (difference between master and PR)?

then we must install the branch version

Your close to the same path I am thinking. My thinking was:
The issue is we can't load the same class in memory in a single JVM as it creates a conflict. There is, however, nothing preventing us from invoking a separate JVM process. We do something similar in sevntu regression against checkstyle.
My plan was to invoke some simple program in our PR copy of Checkstyle through maven and have it generate the results for us that we need. It will pass the information back to our regression-tool's JVM by a file. We don't need the actual class in memory, we just need it's meta-information.
Since it requires an extra file being added to Checkstyle at runtime, we could just ask Checkstyle to add this file into their .gitignore.

@Luolc @romani What is your opinion and preference on how we should get this data from these 3 ways?

[Luolc]

My plan was to invoke some simple program in our PR copy of Checkstyle through maven and have it generate the results for us that we need. It will pass the information back to our regression-tool's JVM by a file.

I think it is a good idea.

[rnveach]

@Luolc So we don't slow down development waiting on final decision, can we work around this by using a hard-coded list of modules (and possibly their packages) and a very simple POJO? Expansion of properties and such will be in Issue #5 .
If we move the retrieve module information to an isolated method, than it shouldn't impact us as much when we make decision. We should only have little code to rewrite than.

[Luolc]

@rnveach Got it. I would refer to the hardcode map in PackageObjectFactory.

[Luolc]

Meanwhile, we need to add some more POJOs for the config generator(i.e. to represent element module, property). These POJOs would be in data package.

[romani]

known meta-data requirements:
eclipse-cs: https://github.com/sevntu-checkstyle/sevntu.checkstyle/tree/master/eclipsecs-sevntu-plugin/src/com/github/sevntu/checkstyle/checks/design
sonar: https://github.com/sevntu-checkstyle/sevntu.checkstyle/blob/master/sevntu-checkstyle-sonar-plugin/src/main/resources/com/github/sevntu/checkstyle/sonar/checkstyle-extensions.xml#L3

Do we need to know if a property/message/token is new/removed (difference between master and PR)?

it is good to know, but lets implement all scenarios step by step. I would focus on 1-2 scenarios and make all component work on them and only after that grow implementation to detect new scenarios and generation of configs to cover.
I do not see discussion on what scenarios (diffs) we should trigger and what set of configs to generate. Please initiate. Choose 1-2 of them to make sooner.

Since it requires an extra file being added to Checkstyle at runtime, we could just ask Checkstyle to add this file into their .gitignore.

if smth is going to be generated by tool in separate clone of repo - no need to be added to ".gitignore".

[romani]

for general question how to grab meta data from checkstyle ......

it might be better to inject into checkstyle repo some functionality(classes) to generate files. All attempts to predict what will be useful for all plugins/tools is hard to analyze for now. So lets keep that functionality out of main repo.
Additional though ..... we could make meta-data generator that is based on java files->parse->AST->process->meta-data , not on reflection in runtime. It will help us to generate not only: field name, field type, default value, but ...... descriptions from javadoc . Descriptions from javadoc is most complicated and demanding point in meta-data generation. We do not need javadoc descriptions in scope of this project, but we will do step forward in this direction.

[rnveach]

@Luolc Please confirm.
We have module component that will grab list of modules to test on from changes in git.
We are moving generating the CS extract to the extract component. Properties are a future issue.
So in terms of just this issue, we are done, right?

[Luolc]

@rnveach Ya I think so.

[rnveach]

Issue is done