Issue #11340: Add ignoreAnnotatedBy property to ParameterNumberCheck #14553

sktpy · 2024-02-26T17:17:57Z

Resolves #11340

Old Doc(site), New Doc(site)

Regression: (Report)

Differences Found (expected):
Deprecated is used as the value for the ignoreAnnotatedBy(selected for the purpose that it would produce some differences being a commonly used annotation)

Removed violations (22): Example (link), rest of the occurrences are similar.

@Deprecated
public static <K, V> ImmutableSortedMap<K, V> of(K k1, V v1, K k2, V v2, K k3, V v3, K k4, V v4) {

Method above contains 8 params, while maximum(by default) allowed are 7,
This is no longer a violation because it is annotated by Deprecated which is specified in ignoreAnnotatedBy property in the patch-config.
This is the intended behaviour.

Diff Regression config: https://gist.githubusercontent.com/sktpy/0c6a38735b3206b86b593a490206e76d/raw/4c033289ef4802e25c473cec5ef58ff49ea8d788/base-config.xml
Diff Regression patch config: https://gist.githubusercontent.com/sktpy/43c4a398aadd414fbe69d75953704597/raw/7f468b2a7e04762d763d7148d81a640dad733091/patch-config.xml

sktpy · 2024-02-26T17:19:39Z

GitHub, generate report

github-actions · 2024-02-26T19:42:27Z

https://checkstyle-diff-reports.s3.us-east-2.amazonaws.com/a62d9ca_2024194155/reports/diff/index.html

sktpy · 2024-02-26T19:43:26Z

GitHub, generate website

github-actions · 2024-02-26T19:46:02Z

https://checkstyle-diff-reports.s3.us-east-2.amazonaws.com/4e3fdbc_2024194458/index.html

https://checkstyle-diff-reports.s3.us-east-2.amazonaws.com/4e3fdbc_2024194458/checks/sizes/parameternumber.html#Properties

sktpy · 2024-02-26T19:50:37Z

...examples/resources/com/puppycrawl/tools/checkstyle/checks/sizes/parameternumber/Example4.txt

+
+// xdoc section -- start
+@Session
+public void ignoredAnnotatedMethod(int a, int b, int c, int d,


Have kept the xdoc examples for ignoreAnnotatedBy in the non-enabled way to not grow this PR more than required, as rest of examples are currently non-enabled and it wouldn't make much sense to have just one example enabled while the rest remain the old way.
All the examples for this check can be enabled at once in a separate PR as part of the ongoing issue to Enable the Examples.

Agreed, but this should at least be compilable. Also, please use something similar to the example provided in the issue, this is a very popular usage when deserializing/serializing. You can just make your own JsonCreator annotation.

sktpy · 2024-02-26T19:55:06Z

src/main/java/com/puppycrawl/tools/checkstyle/checks/sizes/ParameterNumberCheck.java

     */
    private boolean shouldIgnoreNumberOfParameters(DetailAST ast) {
-        // if you override a method, you have no power over the number of parameters


Have removed this comment, because it is more from a philosophical point of view to why the functionality to ignore overridden method was added but does not imparts any helpful technical info.

sktpy · 2024-02-27T16:06:21Z

GitHub, generate website

github-actions · 2024-02-27T16:16:46Z

https://checkstyle-diff-reports.s3.us-east-2.amazonaws.com/e4525b5_2024161531/index.html

https://checkstyle-diff-reports.s3.us-east-2.amazonaws.com/e4525b5_2024161531/checks/sizes/parameternumber.html#Properties

nrmancuso

Very good, few items:

nrmancuso · 2024-02-28T03:36:56Z

...examples/resources/com/puppycrawl/tools/checkstyle/checks/sizes/parameternumber/Example4.txt

+
+// xdoc section -- start
+@Session
+public void ignoredAnnotatedMethod(int a, int b, int c, int d,


Agreed, but this should at least be compilable. Also, please use something similar to the example provided in the issue, this is a very popular usage when deserializing/serializing. You can just make your own JsonCreator annotation.

src/main/java/com/puppycrawl/tools/checkstyle/checks/sizes/ParameterNumberCheck.java

nrmancuso

One more:

src/test/java/com/puppycrawl/tools/checkstyle/checks/sizes/ParameterNumberCheckTest.java

sktpy · 2024-02-28T10:21:51Z

GitHub, generate website

github-actions · 2024-02-28T10:36:15Z

https://checkstyle-diff-reports.s3.us-east-2.amazonaws.com/ebefaf9_2024103356/index.html

https://checkstyle-diff-reports.s3.us-east-2.amazonaws.com/ebefaf9_2024103356/checks/sizes/parameternumber.html#Properties

sktpy · 2024-02-28T11:04:37Z

GitHub, generate website

github-actions · 2024-02-28T11:08:15Z

https://checkstyle-diff-reports.s3.us-east-2.amazonaws.com/be124d3_2024110650/index.html

https://checkstyle-diff-reports.s3.us-east-2.amazonaws.com/be124d3_2024110650/checks/sizes/parameternumber.html#Properties

nrmancuso

Items:

...examples/resources/com/puppycrawl/tools/checkstyle/checks/sizes/parameternumber/Example4.txt

src/xdocs/checks/sizes/parameternumber.xml

sktpy · 2024-02-28T15:01:29Z

GitHub, generate website

github-actions · 2024-02-28T15:40:26Z

https://checkstyle-diff-reports.s3.us-east-2.amazonaws.com/4cc9297_2024153913/index.html

https://checkstyle-diff-reports.s3.us-east-2.amazonaws.com/4cc9297_2024153913/checks/sizes/parameternumber.html#Properties

nrmancuso

Very good, this is a big win for our users. Thanks @sktpy !!

src/it/java/org/checkstyle/suppressionxpathfilter/XpathRegressionParameterNumberTest.java

nrmancuso · 2024-02-28T21:15:49Z

I am in agreement with sentiments at #14553 (comment), namely we can focus on making examples all the "same" when we migrate to actual example test execution. We don't need to make this PR any larger than it is right now.

romani

Items

src/main/java/com/puppycrawl/tools/checkstyle/checks/sizes/ParameterNumberCheck.java

romani · 2024-02-29T02:48:44Z

...ecks/sizes/parameternumber/InputParameterNumberIgnoreAnnotatedByFullyQualifiedClassName.java

+    void method3(int a, int b, int c) {}
+
+    @Deprecated
+    void method4(int a, int b, int c) {} // violation, 'More than 2 parameters (found 3).'


This needs to be discussed.
When use define java.lang.Deprecated he usually does not mean to use match fully and exactly.

It can be instruction to figure out exactly a type not a string values. We already have Checks that does this. By import we can be 100% sure on type.

@nrmancuso , @rnveach , what are your thoughts on this?

This might be helpful here: #14553 (comment)

I think we should go with most simple implementation first (what's in this PR), we can iterate on this as needed. Worst case is that some user has to put both the fully qualified name and the simple annotation name in the config.

Do we have an update here?

I am good to proceed as-is, @rnveach please share your thoughts

I think we should be consistent with how we treat annotation properties like this with other checks. I don't find it friendly to have to specify both class name and FQCN.
https://checkstyle.org/checks/coding/illegaltype.html#IllegalType

We need to make our behavior consistent with other checks and friendly to the user.

We have some check somewhere that requires FQCN and will be smart and reduce it to simple based on the import in the class.
https://checkstyle.org/checks/coding/illegaltype.html
https://checkstyle.org/checks/coding/illegalinstantiation.html

I also feel this shouldn't be delayed if it is going to create almost a breaking change in the fix.

Do we have a resolution here?

I agree we can extend to FQCN later on. Lets have basic implementation for now.
With current traffic of PRs I do not have time to mediate on consistency.

src/main/java/com/puppycrawl/tools/checkstyle/checks/sizes/ParameterNumberCheck.java

rnveach · 2024-03-06T17:04:37Z

src/main/java/com/puppycrawl/tools/checkstyle/checks/sizes/ParameterNumberCheck.java

+    /**
+     * Ignore methods and constructors annotated with the specified annotation(s).
+     */
+    private Set<String> ignoreAnnotatedBy = Set.of();


@nrmancuso @romani We have ignoreOverriddenMethods which also ignores methods based on annotation of Override. Is there some reason we aren't combining these 2 to make the configuration for this check more reasonable?

I was thinking the same, but then I read:
#11340 (comment)

We could deprecate this property, and remove it it the next major version bump.

Do we have a resolution here?

We can leave both for now, let’s create an issue to discuss deprecation of ignoreOverriddenMethods.

@sktpy please open an issue for this, and share the link here so we can move on

ParameterNumberCheck: Deprecation of ignoreOverriddenMethods property #14663

@rnveach please continue review

We can leave both for now

lets not deprecate for now, they should not conflict with each other if user in not doing good job in configuration of them.

src/main/java/com/puppycrawl/tools/checkstyle/checks/sizes/ParameterNumberCheck.java

src/xdocs/checks/sizes/parameternumber.xml.template

romani

ok to merge

rnveach · 2024-03-18T14:48:19Z

https://github.com/sktpy/checkstyle/tree/issue-11340-ignore-annotated-by
This branch is 1 commit ahead of, 52 commits behind

rnveach · 2024-03-18T14:53:11Z

GH made a merge commit by default. I didn't use the dropdown option.

…mberCheck

sktpy · 2024-03-18T15:49:40Z

Rebased over latest master.

sktpy marked this pull request as draft February 26, 2024 17:18

sktpy force-pushed the issue-11340-ignore-annotated-by branch 2 times, most recently from 76980bf to 4e3fdbc Compare February 26, 2024 19:03

sktpy commented Feb 26, 2024

View reviewed changes

This comment was marked as outdated.

Sign in to view

sktpy marked this pull request as ready for review February 26, 2024 20:31

nrmancuso self-requested a review February 26, 2024 23:56

nrmancuso self-assigned this Feb 26, 2024

sktpy force-pushed the issue-11340-ignore-annotated-by branch from 4e3fdbc to e4525b5 Compare February 27, 2024 16:01

nrmancuso requested changes Feb 28, 2024

View reviewed changes

src/test/java/com/puppycrawl/tools/checkstyle/checks/sizes/ParameterNumberCheckTest.java Show resolved Hide resolved

sktpy marked this pull request as draft February 28, 2024 04:53

sktpy force-pushed the issue-11340-ignore-annotated-by branch from e4525b5 to ebefaf9 Compare February 28, 2024 10:21

sktpy force-pushed the issue-11340-ignore-annotated-by branch from ebefaf9 to be124d3 Compare February 28, 2024 10:39

sktpy marked this pull request as ready for review February 28, 2024 11:16

nrmancuso requested changes Feb 28, 2024

View reviewed changes

...examples/resources/com/puppycrawl/tools/checkstyle/checks/sizes/parameternumber/Example4.txt Show resolved Hide resolved

src/xdocs/checks/sizes/parameternumber.xml Outdated Show resolved Hide resolved

src/xdocs/checks/sizes/parameternumber.xml Outdated Show resolved Hide resolved

sktpy force-pushed the issue-11340-ignore-annotated-by branch from be124d3 to 4cc9297 Compare February 28, 2024 15:00

nrmancuso approved these changes Feb 28, 2024

View reviewed changes

src/it/java/org/checkstyle/suppressionxpathfilter/XpathRegressionParameterNumberTest.java Show resolved Hide resolved

nrmancuso assigned romani and unassigned nrmancuso Feb 28, 2024

nrmancuso requested a review from romani February 28, 2024 21:13

romani requested changes Feb 29, 2024

View reviewed changes

sktpy force-pushed the issue-11340-ignore-annotated-by branch from f5a2b14 to 4da8113 Compare February 29, 2024 03:05

sktpy mentioned this pull request Mar 5, 2024

JavadocType: False negative for unknown tag with no description #14573

Open

nrmancuso assigned rnveach and unassigned romani Mar 6, 2024

rnveach requested changes Mar 6, 2024

View reviewed changes

sktpy force-pushed the issue-11340-ignore-annotated-by branch 2 times, most recently from 1caf6f1 to 2298fd5 Compare March 6, 2024 18:02

rnveach assigned romani and unassigned rnveach Mar 7, 2024

sktpy mentioned this pull request Mar 14, 2024

ParameterNumberCheck: Deprecation of ignoreOverriddenMethods property #14663

Open

nrmancuso assigned rnveach and unassigned romani Mar 14, 2024

romani approved these changes Mar 15, 2024

View reviewed changes

romani mentioned this pull request Mar 16, 2024

HideUtilityClassConstructor - Add option to skip validation based on list of annotations #14424

Open

rnveach approved these changes Mar 18, 2024

View reviewed changes

rnveach force-pushed the issue-11340-ignore-annotated-by branch from 2c761c7 to bf078ff Compare March 18, 2024 14:52

Issue checkstyle#11340: Add ignoreAnnotatedBy property to ParameterNu…

ec83b47

…mberCheck

sktpy force-pushed the issue-11340-ignore-annotated-by branch from bf078ff to ec83b47 Compare March 18, 2024 15:48

rnveach merged commit deb1121 into checkstyle:master Mar 20, 2024
112 of 113 checks passed

sktpy mentioned this pull request Mar 20, 2024

Add ignoreMethodAnnotatedBy property to ParameterNumberCheck #11340

Closed

Lmh-java mentioned this pull request May 3, 2024

Issue #14424: Add option to skip validation based on list of annotations #14701

Open

Issue #11340: Add ignoreAnnotatedBy property to ParameterNumberCheck #14553

Issue #11340: Add ignoreAnnotatedBy property to ParameterNumberCheck #14553

Conversation

sktpy commented Feb 26, 2024 • edited

Regression: (Report)

sktpy commented Feb 26, 2024

github-actions bot commented Feb 26, 2024

sktpy commented Feb 26, 2024

github-actions bot commented Feb 26, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

This comment was marked as outdated.

sktpy commented Feb 27, 2024

github-actions bot commented Feb 27, 2024

nrmancuso left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nrmancuso left a comment

Choose a reason for hiding this comment

sktpy commented Feb 28, 2024

github-actions bot commented Feb 28, 2024

sktpy commented Feb 28, 2024

github-actions bot commented Feb 28, 2024

nrmancuso left a comment

Choose a reason for hiding this comment

sktpy commented Feb 28, 2024

github-actions bot commented Feb 28, 2024

nrmancuso left a comment

Choose a reason for hiding this comment

nrmancuso commented Feb 28, 2024

romani left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

nrmancuso Mar 14, 2024 • edited

Choose a reason for hiding this comment

sktpy Mar 14, 2024 • edited

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

romani left a comment

Choose a reason for hiding this comment

rnveach commented Mar 18, 2024

rnveach commented Mar 18, 2024

sktpy commented Mar 18, 2024

sktpy commented Feb 26, 2024 •

edited

nrmancuso Mar 14, 2024 •

edited

sktpy Mar 14, 2024 •

edited