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

Issue #78: extract: grab property info #84

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
5 changes: 5 additions & 0 deletions config/findbugs-exclude.xml
Original file line number Diff line number Diff line change
Expand Up @@ -27,4 +27,9 @@
<Class name="~.*\.Immutable.*"/>
<Bug pattern="NP_METHOD_PARAMETER_TIGHTENS_ANNOTATION"/>
</Match>
<Match>
<!-- We can't change generated source code. -->
<Class name="~.*\.GsonAdapters.*"/>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Where is this file/class? I can't remember.

<Bug pattern="URF_UNREAD_PUBLIC_OR_PROTECTED_FIELD"/>
</Match>
</FindBugsFilter>
2 changes: 1 addition & 1 deletion config/sevntu_suppressions.xml
Original file line number Diff line number Diff line change
Expand Up @@ -4,5 +4,5 @@
"http://www.puppycrawl.com/dtds/suppressions_1_1.dtd">

<suppressions>
<suppress checks="MultipleStringLiteralsExtended" files=".*[\\/]src[\\/]test[\\/]"/>
<suppress checks="MultipleStringLiteralsExtended" files="[\\/]ExtractInfoGeneratorTest.java$|.*[\\/]src[\\/]test[\\/]"/>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

If we are not going to fix this for any custom tests, put path and Test name as suppression.
Example: .*extract[\\/]\w+Test\.java

</suppressions>
2 changes: 1 addition & 1 deletion config/suppressions.xml
Original file line number Diff line number Diff line change
Expand Up @@ -13,7 +13,7 @@
<suppress checks="MagicNumber" files=".*[\\/]src[\\/]test[\\/]"/>
<suppress checks="AvoidStaticImport" files=".*[\\/]src[\\/]test[\\/]"/>
<suppress checks="WriteTag" files=".*[\\/]src[\\/]test[\\/]"/>
<suppress checks="MultipleStringLiterals" files=".*[\\/]src[\\/]test[\\/]"/>
<suppress checks="MultipleStringLiterals" files="[\\/]ExtractInfoGeneratorTest.java$|.*[\\/]src[\\/]test[\\/]"/>
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

same.


<suppress checks="AbstractClassName" files=".*[\\/]data[\\/]"/>
</suppressions>
Original file line number Diff line number Diff line change
Expand Up @@ -19,6 +19,8 @@

package com.github.checkstyle.regression.data;

import java.util.List;

import org.immutables.gson.Gson;
import org.immutables.value.Value;

Expand Down Expand Up @@ -48,11 +50,46 @@ public abstract class ModuleExtractInfo {
*/
public abstract String parent();

/**
* The properties of this module.
* @return the properties of this module
*/
public abstract List<ModuleProperty> properties();

/**
* The full qualified name of this module.
* @return the full qualified name of this module
*/
public String fullName() {
return packageName() + "." + name();
}

/** Represents a property of checkstyle module. */
@Gson.TypeAdapters
@Value.Immutable
public interface ModuleProperty {
/**
* The name of this property.
* @return the name of this property
*/
String name();

/**
* The type of this property.
* The value should be one of the followings:
* - Pattern
* - SeverityLevel
* - boolean
* - Scope
* - double[]
* - int[]
* - String[]
* - String
* - URI
* - AccessModifier[]
* - int
* @return the type of this property
*/
String type();
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -19,19 +19,26 @@

package com.puppycrawl.tools.checkstyle;

import java.beans.PropertyDescriptor;
import java.io.File;
import java.io.IOException;
import java.nio.charset.Charset;
import java.nio.file.Files;
import java.nio.file.StandardOpenOption;
import java.util.ArrayList;
import java.util.Arrays;
import java.util.Comparator;
import java.util.List;
import java.util.Set;
import java.util.TreeSet;

import org.apache.commons.beanutils.PropertyUtils;
import org.junit.Test;

import com.puppycrawl.tools.checkstyle.api.AbstractCheck;
import com.puppycrawl.tools.checkstyle.api.AbstractFileSetCheck;
import com.puppycrawl.tools.checkstyle.checks.javadoc.AbstractJavadocCheck;
import com.puppycrawl.tools.checkstyle.internal.CheckUtil;
import com.puppycrawl.tools.checkstyle.internal.TestUtils;
import com.puppycrawl.tools.checkstyle.utils.ModuleReflectionUtils;

/**
Expand All @@ -40,12 +47,57 @@
* @author LuoLiangchen
*/
public class ExtractInfoGeneratorTest {
/** Modules which do not have global properties to drop. */
private static final List<String> XML_FILESET_LIST = Arrays.asList(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Can we drop this and use the check's class hierarchy to determine the parent?

"TreeWalker",
"Checker",
"Header",
"Translation",
"SeverityMatchFilter",
"SuppressionFilter",
"SuppressWarningsFilter",
"BeforeExecutionExclusionFileFilter",
"RegexpHeader",
"RegexpOnFilename",
"RegexpSingleline",
"RegexpMultiline",
"JavadocPackage",
"NewlineAtEndOfFile",
"UniqueProperties",
"FileLength",
"FileTabCharacter"
);

/** Properties of abstract check. */
private static final Set<String> CHECK_PROPERTIES = getProperties(AbstractCheck.class);

/** Properties of abstract Javadoc check. */
private static final Set<String> JAVADOC_CHECK_PROPERTIES =
getProperties(AbstractJavadocCheck.class);

/** Properties of abstract file-set check. */
private static final Set<String> FILESET_PROPERTIES = getProperties(AbstractFileSetCheck.class);

/** Properties without document. */
private static final List<String> UNDOCUMENTED_PROPERTIES = Arrays.asList(
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We are not documenting properties here.
This is more a list of fields that aren't properties.

"Checker.classLoader",
"Checker.classloader",
"Checker.moduleClassLoader",
"Checker.moduleFactory",
"TreeWalker.classLoader",
"TreeWalker.moduleFactory",
"TreeWalker.cacheFile",
"TreeWalker.upChild",
"SuppressWithNearbyCommentFilter.fileContents",
"SuppressionCommentFilter.fileContents"
);

/**
* Generates the extract info file named as "checkstyle_modules.json".
* @throws IOException failure when generating the file
* @throws Exception failure when generating the file
*/
@Test
public void generateExtractInfoFile() throws IOException {
public void generateExtractInfoFile() throws Exception {
final List<Class<?>> modules = new ArrayList<>(CheckUtil.getCheckstyleModules());
modules.sort(Comparator.comparing(Class::getSimpleName));
final JsonUtil.JsonArray moduleJsonArray = new JsonUtil.JsonArray();
Expand All @@ -62,8 +114,10 @@ public void generateExtractInfoFile() throws IOException {
* Creates Json object for a module from the module class.
* @param clazz the given module class
* @return the Json object describing the extract info of the module
* @throws Exception failure when creating Json object
*/
private static JsonUtil.JsonObject createJsonObjectFromModuleClass(Class<?> clazz) {
private static JsonUtil.JsonObject createJsonObjectFromModuleClass(Class<?> clazz)
throws Exception {
final JsonUtil.JsonObject object = new JsonUtil.JsonObject();

final String name = clazz.getSimpleName();
Expand Down Expand Up @@ -94,6 +148,116 @@ else if (ModuleReflectionUtils.isRootModule(clazz)) {
object.add("interfaces", interfaces);
object.add("hierarchies", hierarchies);

final JsonUtil.JsonArray properties = new JsonUtil.JsonArray();
for (String propertyName : getNecessaryProperties(clazz)) {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Add empty line before for and after } for for.

final JsonUtil.JsonObject property = new JsonUtil.JsonObject();
property.addProperty("name", propertyName);
Arrays.stream(PropertyUtils.getPropertyDescriptors(clazz))
.filter(p -> p.getName().equals(propertyName))
.map(PropertyDescriptor::getPropertyType)
.map(Class::getSimpleName)
.findAny()
.ifPresent(type -> property.addProperty("type", type));
properties.add(property);
}
object.add("properties", properties);

return object;
}

/**
* Gets the necessary properties of a checkstyle module.
* Global properties and undocumented properties are not necessary for us.
* @param clazz the class instance of the given module
* @return a set of the necessary properties of the module
* @throws Exception failure when getting properties
*/
// -@cs[CyclomaticComplexity] many different kinds of module
private static Set<String> getNecessaryProperties(Class<?> clazz)
throws Exception {
final Set<String> properties = getProperties(clazz);
if (hasParentModule(clazz.getSimpleName())) {
if (AbstractJavadocCheck.class.isAssignableFrom(clazz)) {
properties.removeAll(JAVADOC_CHECK_PROPERTIES);
}
else if (ModuleReflectionUtils.isCheckstyleCheck(clazz)) {
properties.removeAll(CHECK_PROPERTIES);
}
}
if (ModuleReflectionUtils.isFileSetModule(clazz)) {
properties.removeAll(FILESET_PROPERTIES);

// override
properties.add("fileExtensions");
}

// undocumented properties are not necessary
properties.removeIf(prop -> UNDOCUMENTED_PROPERTIES.contains(
clazz.getSimpleName() + "." + prop));

final PackageObjectFactory factory = TestUtils.getPackageObjectFactory();
final Object instance = factory.createModule(clazz.getSimpleName());

if (ModuleReflectionUtils.isCheckstyleCheck(clazz)) {
final AbstractCheck check = (AbstractCheck) instance;

final int[] acceptableTokens = check.getAcceptableTokens();
Arrays.sort(acceptableTokens);
final int[] defaultTokens = check.getDefaultTokens();
Arrays.sort(defaultTokens);
final int[] requiredTokens = check.getRequiredTokens();
Arrays.sort(requiredTokens);

if (!Arrays.equals(acceptableTokens, defaultTokens)
|| !Arrays.equals(acceptableTokens, requiredTokens)) {
properties.add("tokens");
}
}

if (AbstractJavadocCheck.class.isAssignableFrom(clazz)) {
final AbstractJavadocCheck check = (AbstractJavadocCheck) instance;

final int[] acceptableJavadocTokens = check.getAcceptableJavadocTokens();
Arrays.sort(acceptableJavadocTokens);
final int[] defaultJavadocTokens = check.getDefaultJavadocTokens();
Arrays.sort(defaultJavadocTokens);
final int[] requiredJavadocTokens = check.getRequiredJavadocTokens();
Arrays.sort(requiredJavadocTokens);

if (!Arrays.equals(acceptableJavadocTokens, defaultJavadocTokens)
|| !Arrays.equals(acceptableJavadocTokens, requiredJavadocTokens)) {
properties.add("javadocTokens");
}
}

return properties;
}

/**
* Gets the properties of a checkstyle module.
* @param clazz the class instance of the given module
* @return a set of the properties of the module
*/
private static Set<String> getProperties(Class<?> clazz) {
final Set<String> result = new TreeSet<>();
final PropertyDescriptor[] map = PropertyUtils.getPropertyDescriptors(clazz);

for (PropertyDescriptor p : map) {
if (p.getWriteMethod() != null) {
result.add(p.getName());
}
}

return result;
}

/**
* Checks whether a module has a parent that may contains global properties.
* @param className the class name of given module
* @return true if the module has a parent
*/
private static boolean hasParentModule(String className) {
return !XML_FILESET_LIST.contains(className) && XML_FILESET_LIST.stream()
.map(name -> name + "Check").noneMatch(name -> name.equals(className));
}
}
Original file line number Diff line number Diff line change
Expand Up @@ -38,6 +38,7 @@
import org.junit.Test;

import com.github.checkstyle.regression.data.ImmutableModuleExtractInfo;
import com.github.checkstyle.regression.data.ImmutableModuleProperty;
import com.github.checkstyle.regression.data.ModuleExtractInfo;

public class ExtractInfoProcessorTest {
Expand Down Expand Up @@ -69,6 +70,14 @@ public void testGetModuleExtractInfosFromReader() throws Exception {
.name(module1)
.packageName(BASE_PACKAGE + ".checks")
.parent("Checker")
.addProperties(ImmutableModuleProperty.builder()
.name("fileExtensions")
.type("String[]")
.build())
.addProperties(ImmutableModuleProperty.builder()
.name("lineSeparator")
.type("String")
.build())
.build();
final String module2 = "EmptyStatementCheck";
final ModuleExtractInfo extractInfo2 = ImmutableModuleExtractInfo.builder()
Expand Down
Original file line number Diff line number Diff line change
Expand Up @@ -39,6 +39,7 @@
import com.github.checkstyle.regression.data.ImmutableGitChange;
import com.github.checkstyle.regression.data.ImmutableModuleExtractInfo;
import com.github.checkstyle.regression.data.ImmutableModuleInfo;
import com.github.checkstyle.regression.data.ImmutableModuleProperty;
import com.github.checkstyle.regression.data.ModuleExtractInfo;
import com.github.checkstyle.regression.data.ModuleInfo;
import com.github.checkstyle.regression.extract.ExtractInfoProcessor;
Expand Down Expand Up @@ -117,6 +118,14 @@ public void testGenerateConfigNodesForValidChanges2() {
.name("NewlineAtEndOfFileCheck")
.packageName(BASE_PACKAGE + ".checks")
.parent("Checker")
.addProperties(ImmutableModuleProperty.builder()
.name("fileExtensions")
.type("String[]")
.build())
.addProperties(ImmutableModuleProperty.builder()
.name("lineSeparator")
.type("String")
.build())
.build();
final List<ModuleInfo> moduleInfos =
ModuleCollector.generate(changes);
Expand Down
13 changes: 13 additions & 0 deletions src/test/resources/checkstyle_modules.json
Original file line number Diff line number Diff line change
Expand Up @@ -12,6 +12,16 @@
"com.puppycrawl.tools.checkstyle.api.FileSetCheck",
"com.puppycrawl.tools.checkstyle.api.Configurable",
"com.puppycrawl.tools.checkstyle.api.Contextualizable"
],
"properties": [
{
"name": "fileExtensions",
"type": "String[]"
},
{
"name": "lineSeparator",
"type": "String"
}
]
},
{
Expand All @@ -26,6 +36,9 @@
"interfaces": [
"com.puppycrawl.tools.checkstyle.api.Configurable",
"com.puppycrawl.tools.checkstyle.api.Contextualizable"
],
"properties": [

]
}
]