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

Conversation

Luolc
Copy link
Collaborator

@Luolc Luolc commented Aug 21, 2017

#78

Processing result: https://gist.github.com/Luolc/241c6e730deff72035abdb8e7feae889

Add property name and type.

Now type could be detected as follows:

  • Pattern
  • SeverityLevel
  • boolean
  • Scope
  • double[]
  • int[]
  • String[]
  • String
  • URI
  • AccessModifier[]
  • int

Comparing to http://checkstyle.sourceforge.net/property_types.html, Pattern is the type of Regexp. Other enums are all seen as string. I haven't find a proper way to handle all the enums currently.

@rnveach
Copy link
Member

rnveach commented Aug 21, 2017

Other enums are all seen as string. I haven't find a proper way to handle all the enums currently.

This is a limitation of Checkstyle.
See:
checkstyle/checkstyle#3542
checkstyle/checkstyle#3543
checkstyle/checkstyle#3254

Now type could be detected as follows:
String

Everything is recognized as String if it isn't identified as one of the others. This doesn't mean it is truly String.

@Luolc Luolc force-pushed the issue78 branch 2 times, most recently from e01cedb to 577d413 Compare August 21, 2017 18:37
@rnveach
Copy link
Member

rnveach commented Aug 22, 2017

@Luolc Until the issues I mentioned above are done, I don't think we can rely fully on the type we get from Checkstyle. Even if we do get all types, we would need more reflection to get enumeration values as they could change between releases or in PR.

Did you plan to use the types gathered and how?

@@ -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

@@ -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.

@@ -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.

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.

@@ -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?

@@ -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.

@rnveach
Copy link
Member

rnveach commented Aug 24, 2017

@Luolc ping

1 similar comment
@rnveach
Copy link
Member

rnveach commented Aug 28, 2017

@Luolc ping

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

2 participants