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

Do not handle local variables for VARIABLE_DEF in AnnotationLocation #6463

Closed
Vampire opened this issue Feb 23, 2019 · 5 comments

Comments

Projects
None yet
3 participants
@Vampire
Copy link
Contributor

commented Feb 23, 2019

With the reasoning from #6416:

Quote from @romani:

All that tokens are not related to original idea of Check Check enforce to locate annotations immediately after documentation block and before target element, annotation should be located on separate line from target element. ... attention to documentation block.
Line wrapping that is possible with annotation usage should be covered by some other Check, I am not ready even tell about possible design for new Check .... live/users will show us what might be valuable in such validations.

AnnotationLocation check for VARIABLE_DEF should only handle field definitions, but no local variable defintions as they have no documentation.

This would also simplify the implementation, because local variable definitions in for-each loops and for-loop-initializers are specially treated anyway, having a single annotation with or without parameters on the same line with the parameter is always allowed though the documentation does not indicate this. This special treatment can also be removed.

If the local variable defintion handling stays, then at least the documentation should be updated to describe the special treatment of local variable definitions in for-each loops and for-loop-initializers for which only a small subset like multiple annotations on the same line are checked.

Documentation should updated anyway, even if the local variable handling gets removed / local variables handling suppressed, the documentation should state that it is only effective on field definitions, not on local variable definitions.

ATTENTION: If users would like to validate location annotation over local variables , new Check should be created. That will have set of options to required to satisfy requirements, wihtout any conflicts with annotations over declaration elements that might have javadoc.

@romani

This comment has been minimized.

Copy link
Member

commented Mar 3, 2019

This would also simplify the implementation, because local variable definitions in for-each loops and for-loop-initializers are specially treated anyway

we should update documentation to clearly state that this Check is only for blocks/statements that might have javadoc. If required, implementation should be adjusted also.

@romani romani added the approved label Mar 3, 2019

Vampire added a commit to Vampire/checkstyle that referenced this issue Mar 8, 2019

Issue checkstyle#6463: Only handle fields for VARIABLE_DEF in Annotat…
…ionLocation check, not local variables

Vampire added a commit to Vampire/checkstyle that referenced this issue Mar 8, 2019

Issue checkstyle#6463: Only handle fields for VARIABLE_DEF in Annotat…
…ionLocation check, not local variables

Vampire added a commit to Vampire/checkstyle that referenced this issue Mar 8, 2019

Issue checkstyle#6463: Only handle fields for VARIABLE_DEF in Annotat…
…ionLocation check, not local variables

Vampire added a commit to Vampire/checkstyle that referenced this issue Mar 9, 2019

Issue checkstyle#6463: Only handle fields for VARIABLE_DEF in Annotat…
…ionLocation check, not local variables

Vampire added a commit to Vampire/checkstyle that referenced this issue Mar 10, 2019

Issue checkstyle#6463: Only handle fields for VARIABLE_DEF in Annotat…
…ionLocation check, not local variables

Vampire added a commit to Vampire/checkstyle that referenced this issue Mar 10, 2019

Issue checkstyle#6463: Only handle fields for VARIABLE_DEF in Annotat…
…ionLocation check, not local variables

Vampire added a commit to Vampire/checkstyle that referenced this issue Mar 11, 2019

Issue checkstyle#6463: Only handle fields for VARIABLE_DEF in Annotat…
…ionLocation check, not local variables

Vampire added a commit to Vampire/checkstyle that referenced this issue Mar 12, 2019

Issue checkstyle#6463: Only handle fields for VARIABLE_DEF in Annotat…
…ionLocation check, not local variables

Vampire added a commit to Vampire/checkstyle that referenced this issue Mar 18, 2019

Issue checkstyle#6463: Only handle fields for VARIABLE_DEF in Annotat…
…ionLocation check, not local variables

Vampire added a commit to Vampire/checkstyle that referenced this issue Mar 18, 2019

Issue checkstyle#6463: Only handle fields for VARIABLE_DEF in Annotat…
…ionLocation check, not local variables

Vampire added a commit to Vampire/checkstyle that referenced this issue Mar 19, 2019

Issue checkstyle#6463: Only handle fields for VARIABLE_DEF in Annotat…
…ionLocation check, not local variables

Vampire added a commit to Vampire/checkstyle that referenced this issue Apr 10, 2019

Issue checkstyle#6463: Only handle fields for VARIABLE_DEF in Annotat…
…ionLocation check, not local variables

Vampire added a commit to Vampire/checkstyle that referenced this issue Apr 10, 2019

Issue checkstyle#6463: Only handle fields for VARIABLE_DEF in Annotat…
…ionLocation check, not local variables

Vampire added a commit to Vampire/checkstyle that referenced this issue Apr 11, 2019

Issue checkstyle#6463: Only handle fields for VARIABLE_DEF in Annotat…
…ionLocation check, not local variables

Vampire added a commit to Vampire/checkstyle that referenced this issue Apr 11, 2019

Issue checkstyle#6463: Only handle fields for VARIABLE_DEF in Annotat…
…ionLocation check, not local variables

Vampire added a commit to Vampire/checkstyle that referenced this issue May 12, 2019

Issue checkstyle#6463: Only handle fields for VARIABLE_DEF in Annotat…
…ionLocation check, not local variables
@rnveach

This comment has been minimized.

Copy link
Member

commented May 15, 2019

@romani This feature was requested for by Apache Apex in #3117 ( with 21cad8b ) though it seems they are still on old 6.15.
https://github.com/apache/apex-core/blob/d17f464fcaf19778e2f8edbe2b03419151558068/pom.xml#L406

@Vampire

This comment has been minimized.

Copy link
Contributor Author

commented May 15, 2019

As far as I can see, that ticket requested the special handling, but the outcome will be the same, as relevant occurrences are now not handled at all anymore.

Vampire added a commit to Vampire/checkstyle that referenced this issue May 15, 2019

Issue checkstyle#6463: Only handle fields for VARIABLE_DEF in Annotat…
…ionLocation check, not local variables

Vampire added a commit to Vampire/checkstyle that referenced this issue May 15, 2019

Issue checkstyle#6463: Only handle fields for VARIABLE_DEF in Annotat…
…ionLocation check, not local variables
@romani

This comment has been minimized.

Copy link
Member

commented May 15, 2019

There will be only minor breaking compatibility, that Check stop to violate code on local variables that have same line annotation, but user wants it be to be separate line. New Check should cover this.
Coverage of all possible cases by one Check with numerous properties is error prone, we already learned that lessson.

@romani romani closed this in #6654 May 18, 2019

romani added a commit that referenced this issue May 18, 2019

@romani romani added the bug label May 18, 2019

@romani romani added this to the 8.21 milestone May 18, 2019

@romani

This comment has been minimized.

Copy link
Member

commented May 18, 2019

fix is merged.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.