Issue #108: added ForbidAnnotationElementValueCheck #476
Conversation
There was a problem hiding this comment.
Want to rename this to isSingleElementAnnotationWithForbiddenValue?
lets keep as it is, looks ok now.
You want me to run regression on this check?
yes. lets test our ability to remove moratorium
What property values do you wish me to use besides empty check?
- forbid - "Test(expected)"
- some text of array value of annotation, probably at "SuppresWarning" , it will give some violations for sure.
| if (getAnnotationName(ast).equals(annotationName)) { | ||
| if (DEFAULT_ELEMENT_NAME.equals(elementName) && isSingleElementAnnotation(ast)) { | ||
| log(getSingleElementWithForbiddenValue(ast), MSG_KEY, DEFAULT_ELEMENT_NAME, | ||
| annotationName); |
There was a problem hiding this comment.
please move getSingleElementWithForbiddenValue out of log.
Whole logic is in this method, it should not be hidden.
log line should be similar to next log line, by structure.
| * Sets Annotation Name Check property. | ||
| * | ||
| * @param annotationName | ||
| * THe annotation name. |
There was a problem hiding this comment.
typo.
can we make it single line
| * Sets Annotation Element Check property. | ||
| * | ||
| * @param elementName | ||
| * The annotation element name. |
| * </p> | ||
| * | ||
| * <pre> | ||
| * @Bean(name = AnnotationConfigUtils.SCHEDULED_ANNOTATION_PROCESSOR_BEAN_NAME) |
There was a problem hiding this comment.
lets be more practical in examples and do real users problems:
"usage of "expected" parameter in annotation Test"
in web we could find smth else.
| * </p> | ||
| * | ||
| * <pre> | ||
| * @Endorsers({"Children", "Unscrupulous dentists"})<br> |
There was a problem hiding this comment.
example should be close to real life, if nothing come to mind - make example base on SuppressWarning annotation.
There was a problem hiding this comment.
@romani What do you think of changing these default values?
As I was working to start regression, I noticed some of them didn't make sense for easy use and found myself having to define these explicitly.
<module name="ForbidAnnotationElementValueCheck" />
<module name="ForbidAnnotationElementValueCheck">
<property name="annotationName" value="Test" />
<property name="elementName" value="expected" />
<property name="forbiddenElementValueRegexp" value=".*" />
</module>
<module name="ForbidAnnotationElementValueCheck">
<property name="annotationName" value="SuppressWarnings" />
<property name="elementName" value="value" />
<property name="forbiddenElementValueRegexp" value=".*unchecked.*" />
</module>
Regression found NPE on:
@javax.annotation.ParametersAreNonnullByDefault
package com.google.common.reflect;
Caused by: java.lang.NullPointerException
at com.puppycrawl.tools.checkstyle.checks.annotation.ForbidAnnotationElementValueCheck.getAnnotationName(ForbidAnnotationElementValueCheck.java:373)
at com.puppycrawl.tools.checkstyle.checks.annotation.ForbidAnnotationElementValueCheck.visitToken(ForbidAnnotationElementValueCheck.java:174)
| private String annotationName; | ||
|
|
||
| /** Forbidden annotation element name. */ | ||
| private String elementName; |
There was a problem hiding this comment.
We should have default element name be DEFAULT_ELEMENT_NAME?
There was a problem hiding this comment.
I do not understamd question. Please change code as you want .
| private static final String DEFAULT_ELEMENT_NAME = "value"; | ||
|
|
||
| /** Precompiled forbidden element value pattern. */ | ||
| private Pattern forbiddenElementValueRegexp = Pattern.compile("^$"); |
There was a problem hiding this comment.
We should have default regexp be a match everything? (IE: .*)
Current expression matches empty value, which shouldn't normally happen.
There was a problem hiding this comment.
Check state was far from completed, so please change default to reasonable smth
| || currentNode.getType() == TokenTypes.ANNOTATION_ARRAY_INIT) { | ||
| final String elementValue = getSingleElementValue(currentNode); | ||
|
|
||
| if (forbiddenElementValueRegexp.matcher(elementValue).matches()) { |
There was a problem hiding this comment.
This seems to make more sense as a find than as a matches.
| final String elementValue = getElementValue(memberValuePair); | ||
| final Matcher elementValueMatcher = forbiddenElementValueRegexp.matcher(elementValue); | ||
|
|
||
| return getElementName(memberValuePair).equals(elementName) && elementValueMatcher.matches(); |
There was a problem hiding this comment.
Same here, find instead of matches.
|
I would make this Check to catch "Test(expected) " by default. |
|
All requests were addressed. Default of check was changed. |
|
Regression: |
|
awesome work, unfortunately bad code is in checkstyle and sevntu .... even I tried to not make that code appear. Fortunately now it is automated. @rnveach , please add this Check to confugrations in main project to enforce good practice. I am going to make new release of senvtu now. |
Issue #108
Original code: https://github.com/drozzds/sevntu.checkstyle/commit/cca522efd875dcc913e6169d3c3ff963a09e1cf3
Original discussion: https://groups.google.com/forum/#!msg/sevntu-checkstyle/5MFHVlFTmz8/YBuBK2IonZsJ
Original PR: #265
Only changes are really formatting, field/method renames, and fixed violations.
Numbers from last review:
getSingleElementWithForbiddenValue(new name) which goes against one of the check properties. Want to rename this toisSingleElementAnnotationWithForbiddenValue?