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

Since version is determined incorrectly by property macros at returncount.xml.template #13934

Closed
romani opened this issue Oct 23, 2023 · 8 comments

Comments

@romani
Copy link
Member

romani commented Oct 23, 2023

Detected at https://github.com/checkstyle/checkstyle/pull/13932/files#r1368105661

Expected since is from setter, value should be 3.4.

Same problem is at https://github.com/checkstyle/checkstyle/pull/13971/files#r1385112133
For AtclauseOrder

@relentless-pursuit
Copy link

relentless-pursuit commented Oct 24, 2023

When i changed the @since of the format propertyNmae to something random like 5.0, but it was still drawing @since from the check i.e. 3.2

/**
 * <p>
 * Restricts the number of return statements in methods, constructors and lambda expressions.
 *  .....
 * </li>
 * </ul>
 *
 * @since 3.2
 */
/**
 * Setter to specify method names to ignore.
 *
 * @param pattern a pattern.
 * @since 5.0
*/
public void setFormat(Pattern pattern) {
  this.format = pattern;
    }

Some logs to set the context:

propertyName inside else-if: format
sinceVersion inside else-if: 3.2

And, the reason why the @since 3.2 is tied to the propertyName which is format here is because of the following piece of code of getSinceVersion in SiteUtil.java:

 else if (SUPER_CLASS_PROPERTIES_JAVADOCS.containsKey(propertyName)
                || TOKENS.equals(propertyName)
                || JAVADOC_TOKENS.equals(propertyName)) {
                ....
}

And, the propertyName inside matches with propertyName of ReturnCountCheck.java i.e. format.
Contents of SUPER_CLASS_PROPERTIES_JAVADOCS:

Key: charset
Key: AbstractHeaderCheck
Key: AbstractAccessControlNameCheck
Key: AbstractJavadocCheck
Key: applyToPackage
Key: violateExecutionOnNonTightHtml
**Key: format**
Key: javadocTokens
Key: applyToPublic
Key: tabWidth
Key: fileContents
Key: applyToPrivate
Key: AbstractNameCheck
Key: AbstractFileSetCheck
Key: fileExtensions
Key: headerFile
Key: AbstractParenPadCheck
Key: header
Key: applyToProtected
Key: option

Maybe, this requires some additional logic to detach @since of setter with @since of the check. One way could be change the propertyName. Also, i am confused why format belonging to AbstractClassNameCheck introduce with 3.2 differs from format belonging to ReturnCountCheck was introduce in 3.4

@Rohanraj123
Copy link
Contributor

Rohanraj123 commented Oct 24, 2023

@relentless-pursuit Even in the getSinceVersion method in SiteUtil.java file returning the sinceversion it takes from javadoc that has been created. So why do not just change the value of since in returncount.xml and returncount.xml.template. Please letme know if i am wrong or i have wrong understanding. Looking for constructive feedbacks.

@relentless-pursuit
Copy link

relentless-pursuit commented Oct 24, 2023

@Rohanraj123 ,
If you change the version of setFromat in the below code:

  /**
    * Setter to specify method names to ignore.
    *
    * @param pattern a pattern.
    * @since 3.4
    */
   public void setFormat(Pattern pattern) {
       this.format = pattern;
   }

And, re-run the test cases, it will draw version 3.2 which is the version of the check.

If you read the original issue attached above, while migrating, the version of the format changed from 3.4 (original) to 3.2. The current issue to me is to revert back to original i.e. 3.4. And, not changing the version of the check itself.

@Rohanraj123
Copy link
Contributor

@Rohanraj123 , If you change the version of setFromat in the below code:

  /**
    * Setter to specify method names to ignore.
    *
    * @param pattern a pattern.
    * @since 3.4
    */
   public void setFormat(Pattern pattern) {
       this.format = pattern;
   }

And, re-run the test cases, it will draw version 3.2 which is the version of the check.

If you read the original issue attached above, while migrating, the version of the format changed from 3.4 (original) to 3.2. The current issue to me is to revert back to original i.e. 3.4. And, not changing the version of the check itself.

Ok thanks to give me better clearance @relentless-pursuit Sir

@romani
Copy link
Member Author

romani commented Oct 24, 2023

We need to update code at

else if (SUPER_CLASS_PROPERTIES_JAVADOCS.containsKey(propertyName)

To keep this collection to be checked by class first, and then by property name to make sure we can find "format" only in specific Checks.

ReturnCount does not have parent class that has some specific properties

public final class ReturnCountCheck extends AbstractCheck {

@rnveach
Copy link
Member

rnveach commented Oct 26, 2023

We need to update code at SUPER_CLASS_PROPERTIES_JAVADOCS

This does not seem like a good "override", as not all properties will match the module's since version.

It sort of made sense to do this manually for tokens since the automated process I used to create these versions could not identify when there was customizable tokens or not. Since this is being automated, this will cause a wrong version to be displayed if a non-customizable token is now converted to be customizable.

@rnveach
Copy link
Member

rnveach commented Oct 26, 2023

else if (SUPER_CLASS_PROPERTIES_JAVADOCS.containsKey(propertyName)

This code is wrong and needs to be fixed.

One of the values in the map is EmptyForIteratorPadCheck and option. However, option already has a javadoc with a @since. It is wrong that we are pulling the @since version from the class. It should be pulled from the setter.

Setter:

Edit:
Technically these are wrong as well.

|| TOKENS.equals(propertyName)
|| JAVADOC_TOKENS.equals(propertyName)) {

@romani
Copy link
Member Author

romani commented Nov 20, 2023

issue is resolved

@romani romani closed this as completed Nov 20, 2023
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants