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

JavadocVariableCheck: explain token in documentation #12134

Open
rnveach opened this issue Aug 29, 2022 · 4 comments
Open

JavadocVariableCheck: explain token in documentation #12134

rnveach opened this issue Aug 29, 2022 · 4 comments

Comments

@rnveach
Copy link
Member

rnveach commented Aug 29, 2022

Identified at #3955 ,

https://checkstyle.org/config_javadoc.html#JavadocVariable_Properties

There are no examples or explanation from what ENUM_CONSTANT_DEF gives you in this check.
It is enabled by default, but what happens if you disable it, if its possible. It is also not clear how it can be disabled in configuration, which I assume involves setting the property to an empty string.

We should examine what this token does, if it should remain optional, and update the xdoc based on the findings.

@rnveach
Copy link
Member Author

rnveach commented Aug 29, 2022

This was not handled/discussed in #7602 (documentation update) .

Since the check first introduced in 5922229 , VARIABLE_DEF was a default token alone. It was in 6125bef that ENUM_CONSTANT_DEF was added. At this time the tokens were only specified as default.

Acceptable tokens was added at abe2b20 and both tokens were specified as default and acceptable. However, there was no definition of required at the time. Truthfully, there was never any required tokens by default as seen at:

.

It wasn't until Issue #655 where getRequiredTokens was enforced to be specified. At d56a2a3 only VARIABLE_DEF was added as a required token. The only comment was that a user requested to only be able to disable one and not the other. There was no discussion about this change in the PR at #1723 .

The 2 tokens control validating fields and validating enumeration constants respectively.

Tokens like this, where a single token is listed in the acceptable and default required documentation tables can be "removed". The only way to do this is to define properties as the list of required tokens. The required tokens for this check is VARIABLE_DEF.

@rnveach
Copy link
Member Author

rnveach commented Aug 29, 2022

IMO, I think this check has expanded such that it should be customizable to disable/enable either one of the tokens or both. It is like JavadocMethod where multiple token combinations can be used depending on what you want to validate. In addition, our table documentation doesn't make it clear what the other token to add is to disable the other. We specifically have to add documentation stating how because it is not intuitive.

I think we should revert this back to no tokens are required. All tokens should be acceptable and default.

@romani Let me know if this can be approved. I don't see this as breaking compatibility.

@romani
Copy link
Member

romani commented Aug 31, 2022

I think we should revert this back to no tokens are required. All tokens should be acceptable and default.

Yes. To let user create one instance for class fields and another for enum fields.

Can you describe this by CLI ?
To visually show that certain requirements are not possible to archive.

Edit from rnveach: If approved, romani considers this breaking compatibility.

@rnveach
Copy link
Member Author

rnveach commented Aug 31, 2022

$ cat TestClass.java
public class TestClass {
    public int field;

    public enum TestEnum {
      MY_CONST;
    }
}

$ cat TestConfig.xml
<?xml version="1.0"?>
<!DOCTYPE module PUBLIC
          "-//Puppy Crawl//DTD Check Configuration 1.3//EN"
          "https://checkstyle.org/dtds/configuration_1_3.dtd">

<module name="Checker">
    <property name="charset" value="UTF-8"/>

    <module name="TreeWalker">
<module name="JavadocVariable">
  <property name="id" value="Default" />
</module>
<module name="JavadocVariable">
  <property name="id" value="FieldOnly" />
  <property name="tokens" value="VARIABLE_DEF" />
</module>
<module name="JavadocVariable">
  <property name="id" value="AttemptEnumConstantOnly" />
  <property name="tokens" value="ENUM_CONSTANT_DEF" />
</module>
    </module>
</module>

$ java -jar checkstyle-10.3.2-all.jar -c TestConfig.xml TestClass.java
Starting audit...
[ERROR] TestClass.java:2:5: Missing a Javadoc comment. [AttemptEnumConstantOnly]
[ERROR] TestClass.java:2:5: Missing a Javadoc comment. [Default]
[ERROR] TestClass.java:2:5: Missing a Javadoc comment. [FieldOnly]
[ERROR] TestClass.java:5:7: Missing a Javadoc comment. [AttemptEnumConstantOnly]
[ERROR] TestClass.java:5:7: Missing a Javadoc comment. [Default]
Audit done.
Checkstyle ends with 5 errors.

As shown above, default validates both fields and enum constants. It is possibly to validate only fields, if I wish to ignore enum constants. However, when attempting to only validate enum constants, it is still validating fields showing it is not possible to only validate enum constants.

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

No branches or pull requests

2 participants