RequiredParameterForAnnotationCheck

102 views
Skip to first unread message

Даниил Ярославцев

unread,
Aug 23, 2014, 2:41:47 PM8/23/14
to sevntu-c...@googlegroups.com

Даниил Ярославцев

unread,
Aug 23, 2014, 3:59:07 PM8/23/14
to sevntu-c...@googlegroups.com
Hello, Clebert

After review, we have some minor objections to your code, could you, please, resolve them?

1.

String names[] = getAnnotationParameters(aAST);

for (String p : names) {
    if (requiredParameters.contains(p)) {
        found++;
    }
}

I think, it would be better to calculate set of actually present required parameters and then just compare it with desired set with equals().
Like this:
if(!getRequiredAnnotationParameters(aAST).equals(this.requiredParameters)) {
   log(...)
}

So, it would be no need to count required annotation parameters.

2. There should be no variable names like 'p', 'text' or 'found'. Every variable name should exactly explain what is it for.
e.g.:
p --> annotationParameterName 
text --> requiredParameter
found --> foundRequiredParametersCount

3.  Please format your code with our Formatter for Checkstyle and check it with our 'Checkstyle for Checkstyle' configuration. See more details at https://github.com/sevntu-checkstyle/sevntu.checkstyle/wiki/Java-code-Formater-and-Checkstyle-configuration-for-development

4. As far as I remember there is a Guava dependency in Checkstyle. So, you could use Guava's Joiner to let code 

Iterator<String> iter = requiredParameters.iterator();
while (iter.hasNext()) {
    String text = iter.next();
    propertiesText.append(text);
    if (iter.hasNext()) {
        propertiesText.append(",");
    }
}

look like this:

Joiner.on(", ").join(requiredParameters);

Даниил Ярославцев

unread,
Aug 23, 2014, 4:14:33 PM8/23/14
to sevntu-c...@googlegroups.com
Clebert, could you, please, also update files:

and also

With lines for your new Check in order to let Eclipse Checkstyle plugin work with your check properly.
There is also a config file for Sonar Checkstyle Plugin, please, update it with configuration for your Check, too.

Just search the key 'ForbidAnnotationCheck' in all that files to see an examples of how and where to write configs for annotations-related checks.
Feel free to ask any questions, we will help you where needed.

Additionally, you could test your Check for Exceptions and false-positives on opensource projects by making your own sevntu-checkstyle build containing your check using this manual.
With such a custom build, you could start using your check before we will merge it to main repo and it appear in next releases.

Clebert Suconic

unread,
Aug 23, 2014, 6:07:38 PM8/23/14
to sevntu-c...@googlegroups.com


On Saturday, August 23, 2014 3:59:07 PM UTC-4, Даниил Ярославцев wrote:
Hello, Clebert

After review, we have some minor objections to your code, could you, please, resolve them?

1.

String names[] = getAnnotationParameters(aAST);

for (String p : names) {
    if (requiredParameters.contains(p)) {
        found++;
    }
}

I think, it would be better to calculate set of actually present required parameters and then just compare it with desired set with equals().
Like this:
if(!getRequiredAnnotationParameters(aAST).equals(this.requiredParameters)) {
   log(...)
}


So, it would be no need to count required annotation parameters.





It would diverge semantic from what I intended. My intention was to not force the order or keep it specifically. Say If I wanted to force parameter name on @Parameterized, someone could use a third parameter without a problem. Actually that shows on the tests I wrote.

It;s really about counting than the order. If you set name on @Parameterized (as my example), people would be able to use extra parameters on the annotation.

I can review the semantics if more people find it to be a better use. (It's not on my experience.. but I'm open to discuss it).
 
 
2. There should be no variable names like 'p', 'text' or 'found'. Every variable name should exactly explain what is it for.
e.g.:
p --> annotationParameterName 
text --> requiredParameter
found --> foundRequiredParametersCount

Ok, I will do it
 
3.  Please format your code with our Formatter for Checkstyle and check it with our 'Checkstyle for Checkstyle' configuration. See more details at https://github.com/sevntu-checkstyle/sevntu.checkstyle/wiki/Java-code-Formater-and-Checkstyle-configuration-for-development

will do it
 

4. As far as I remember there is a Guava dependency in Checkstyle. So, you could use Guava's Joiner to let code 

Iterator<String> iter = requiredParameters.iterator();
while (iter.hasNext()) {
    String text = iter.next();
    propertiesText.append(text);
    if (iter.hasNext()) {
        propertiesText.append(",");
    }
}

look like this:

Joiner.on(", ").join(requiredParameters);


You learn a new thing every day :) Will look at it! 

Clebert Suconic

unread,
Aug 23, 2014, 6:16:35 PM8/23/14
to sevntu-c...@googlegroups.com
Additionally, you could test your Check for Exceptions and false-positives on opensource projects by making your own sevntu-checkstyle build containing your check using this manual.
With such a custom build, you could start using your check before we will merge it to main repo and it appear in next releases.



I'm not sure I understand what that means?

I'm using a clone of that now on HornetQ: 

Clebert Suconic

unread,
Aug 25, 2014, 11:22:10 AM8/25/14
to sevntu-c...@googlegroups.com
I just reopened the PR with the requested changes... 


I couldn't use the eclipse plugin here as I have some limitations currently on my environment. but I believe everything is correct. if you guys could take a look please? and I also have some limitations on my time for the next 2 weeks.


I've tried to submit this to checkstyle and Roman Ivanov told me to commit it to this "sandbox". I'm not really sure I understand if sevntu is an extension, a sandbox of checkstyle? I'm a bit lost on the relationship with checkstyle here? couldn't find much information on the README.



thanks,


Clebert

Roman Ivanov

unread,
Aug 25, 2014, 3:06:44 PM8/25/14
to sevntu-c...@googlegroups.com
Hi Clebert,

I've tried to submit this to checkstyle and Roman Ivanov told me to commit it to this "sandbox". I'm not really sure I understand if sevntu is an extension, a sandbox of checkstyle? I'm a bit lost on the relationship with checkstyle here? couldn't find much information on the README

Даниил Ярославцев

unread,
Aug 30, 2014, 8:17:54 PM8/30/14
to sevntu-c...@googlegroups.com
Hello, Clebert!

Sorry, for delay,

1. Please remove redundant return at ForbidAnnotationCheck.java, line 25 as it has appeared after your changes.
2. There is a minor typo in Check JavaDoc:
Marks a given parameter as required for a giveN annotation
3. Please update Joiner.on(",") to Joiner.on(", ") to let printed error be better formatted.
4. From /eclipsecs-sevntu-plugin/src/com/github/sevntu/checkstyle/checks/annotation/checkstyle-metadata.properties:

RequiredParameterForAnnotation.name        = Required Parameters on Annotation
RequiredParameterForAnnotation.desc     = Enforce the use of configured parameters on a given annotation
ForbidAnnotation.annotationName         = The name of the annotation where we are enforcing the parameter
ForbidAnnotation.requiredParameters        = Set of parameters that are required on the configured annotation. They can be specified on any order and you may use additional parameters, but these parameters need to be filled.

please update 'ForbidAnnotation' to 'RequiredParameterForAnnotation' in order to let description for check properties be visible in plugin's UI.
Also, please update description for 'requiredParameters' property in order to contain words about several required annotations could be written separated by comma.

Ans also, just curious: what app generates ".metadata" files you've added to .gitignore?

All another looks good, I've tested your check on Spring and Hibernate sources, seems to be no exceptions and false-positives. So I am OK to merge your code to sandbox repo after aforesaid fixes.

Clebert Suconic

unread,
Sep 2, 2014, 12:12:16 PM9/2/14
to sevntu-c...@googlegroups.com
Hey thanks... sorry for the delay also, it was a big holiday in US this monday (Labor day) and I took the weekend off.


I have made these individual commits on a branch https://github.com/clebertsuconic/sevntu.checkstyle/commits/master-work, and I have rebased/squased on the PR:


You will find these invidual commits to help you double check the changes accordingly, but the PR is sent as a single commit (what would make sense better in a longer term)


Follows my comments inline.


On Saturday, August 30, 2014 8:17:54 PM UTC-4, Даниил Ярославцев wrote:
Hello, Clebert!

Sorry, for delay,

1. Please remove redundant return at ForbidAnnotationCheck.java, line 25 as it has appeared after your changes.

 
2. There is a minor typo in Check JavaDoc:
Marks a given parameter as required for a giveN annotation

 
3. Please update Joiner.on(",") to Joiner.on(", ") to let printed error be better formatted.

I've also found a little bug here. The output parameters were not sorted due to the hashmap use.
The commit here is also fixing that, among improving formatting




 
4. From /eclipsecs-sevntu-plugin/src/com/github/sevntu/checkstyle/checks/annotation/checkstyle-metadata.properties:

RequiredParameterForAnnotation.name        = Required Parameters on Annotation
RequiredParameterForAnnotation.desc     = Enforce the use of configured parameters on a given annotation
ForbidAnnotation.annotationName         = The name of the annotation where we are enforcing the parameter
ForbidAnnotation.requiredParameters        = Set of parameters that are required on the configured annotation. They can be specified on any order and you may use additional parameters, but these parameters need to be filled.

please update 'ForbidAnnotation' to 'RequiredParameterForAnnotation' in order to let description for check properties be visible in plugin's UI.
Also, please update description for 'requiredParameters' property in order to contain words about several required annotations could be written separated by comma.


 
Ans also, just curious: what app generates ".metadata" files you've added to .gitignore?


I opened Eclilpse with the worspace at the root of the project, that made eclipse generating the .metadata.

Clebert Suconic

unread,
Sep 9, 2014, 12:36:02 PM9/9/14
to sevntu-c...@googlegroups.com
any updates? I have covered all the changes.

--
You received this message because you are subscribed to a topic in the Google Groups "Sevntu Checkstyle" group.
To unsubscribe from this topic, visit https://groups.google.com/d/topic/sevntu-checkstyle/YibCclEYexs/unsubscribe.
To unsubscribe from this group and all its topics, send an email to sevntu-checkst...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.



--
Clebert Suconic
http://community.jboss.org/people/clebert...@jboss.com
http://clebertsuconic.blogspot.com

Даниил Ярославцев

unread,
Sep 10, 2014, 7:41:26 PM9/10/14
to sevntu-c...@googlegroups.com
Hello, Clebert

Sorry for delay, had too lot of pull requests this week..

Could you, please, prepare a custom build of our sandbox project containing your check (please use p.1-4 from this manual) and share it here to let us test it on real sources?
More strictly, we need from you compiled eclipsecs-sevntu-plugin_1.11.X.jar containing your check in order to put it into our testing Eclipse instances and launch your check over our testing sources and some huge opensource projects.

In the same time, you will be able to use a custom build with your check until we release new project version containing your update. 

Clebert Suconic

unread,
Sep 10, 2014, 8:49:43 PM9/10/14
to sevntu-c...@googlegroups.com
From a previous email I thought this was done by some of you already...  and all was needed was to merge the PR after these changes.

I will look into finding time later this week / weekend. but if someone else have a way to do this verification I would appreciate.. since I would need to build the environment for that here. 

--
You received this message because you are subscribed to a topic in the Google Groups "Sevntu Checkstyle" group.
To unsubscribe from this topic, visit https://groups.google.com/d/topic/sevntu-checkstyle/YibCclEYexs/unsubscribe.
To unsubscribe from this group and all its topics, send an email to sevntu-checkst...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

Roman Ivanov

unread,
Sep 13, 2014, 10:20:42 AM9/13/14
to sevntu-c...@googlegroups.com
Hi Clebert,

while Daniil is on vacation I will mentor you.

1) As build creation is problem for, I could do it myself.
What IDE are you using ? I am just curious how you are going to use Check after completion.

Code review : 


1) Please keep separate changes in separate branches or squash all commits in one.
It very hard to see your changes.

rForAnnotationCheck.java

1) 
* &lt;module name="RequiredParameterForAnnotation"&gt; &lt;property name="annotationName"

Please format, the configuration.

2) please remove "* </p>" as it cause an error why compiling on  java8

3) what is a reason of empty ctor "public RequiredParameterForAnnotationCheck()" ?

4) 
> Otherwise parameter names will be printed on a random order

so you need order only for logging, TreeSet usage will be a better approach.
You do not validate order of parameters , so LinkedHashSet is not used as intended.

5) 
> // cloning the initial hashMap

I do not like that approach. clone whole collection on each annotation is too much actions.

Please do code like :

String parameterNames[] = getAnnotationParameters(aAST);

for (String lookupParameter : parameterNames) {
 if (!requiredParameters.contains(lookupParameter)
       String requiredParametersAsString = Joiner.on(", ").join(
            missingParameters);
       log(aAST, MSG_KEY, this.annotationName,
            requiredParametersAsString);
}

6) 
I need update in JavaDoc to have examples of code.

7) to make a build I need changes in other files on our sandbox project , see "changed files" as example

8) 
please create UTs+Inputs for annotations on methods arguments, interfaces, enums , any other place where we could put annotation .....
Testing should be full .

thanks,
Roman Ivanov

Clebert Suconic

unread,
Sep 15, 2014, 10:12:06 AM9/15/14
to sevntu-c...@googlegroups.com
I have my changes on a single commit at https://github.com/clebertsuconic/sevntu.checkstyle/commits/master


the master-work branch was just a way to communicate with daniil.yaroslavtsev about the multiple changes he had requested me before. Each commit represents one item from his last post while master (the one where the PR is sent) has a single squashed commit.



I'm planning using on maven, I'm not using it within the IDE. although I'm using Idea.


(I had a lot of issues on making the project to work on Eclipse, and it was automatic with Idea..)


I will work on this during the next few days. Not that I will spend a few days on it.. it's a simple task.. just that I need to find the time to do it either today or tomorrow).



thanks,


Clebert

--
You received this message because you are subscribed to a topic in the Google Groups "Sevntu Checkstyle" group.
To unsubscribe from this topic, visit https://groups.google.com/d/topic/sevntu-checkstyle/YibCclEYexs/unsubscribe.
To unsubscribe from this group and all its topics, send an email to sevntu-checkst...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

RomanIvanov

unread,
Sep 22, 2014, 7:29:45 PM9/22/14
to sevntu-c...@googlegroups.com
Do you need help?
Just checking a progress.

Clebert Suconic

unread,
Sep 22, 2014, 8:38:30 PM9/22/14
to sevntu-c...@googlegroups.com
It's just that I'm flooded with my activities at work.. thanks for checking.


I will make some time this week.

On Mon, Sep 22, 2014 at 7:29 PM, RomanIvanov <romani...@gmail.com> wrote:
Do you need help?
Just checking a progress.
--
You received this message because you are subscribed to a topic in the Google Groups "Sevntu Checkstyle" group.
To unsubscribe from this topic, visit https://groups.google.com/d/topic/sevntu-checkstyle/YibCclEYexs/unsubscribe.
To unsubscribe from this group and all its topics, send an email to sevntu-checkst...@googlegroups.com.
For more options, visit https://groups.google.com/d/optout.

Andrew Uljanenko

unread,
Feb 28, 2015, 2:27:16 PM2/28/15
to sevntu-c...@googlegroups.com
Extended UT's and a little fixed code.
Commit
checkstyle-tester

Roman Ivanov

unread,
Mar 3, 2015, 5:52:07 PM3/3/15
to sevntu-c...@googlegroups.com
Hi Andrew,

1)
> AnnotationAbstract.java

please remove that class, no parents are required for new check. All methods of this class looks like good candidates for static methods.
It might be reasonable to move them to Utils class.

2)
> ForbidAnnotationCheck.java
please remove any changes in that Check, I expect chnages only at RequiredParameterForAnnotationCheck.java

3) 
/checkstyle-extensions.xml
description tag should have the same description that is Javadoc.

thanks,
Roman Ivanov

Roman Ivanov

unread,
Mar 3, 2015, 7:55:23 PM3/3/15
to sevntu-c...@googlegroups.com
Hi Andrew,

In your checkstyle-tester I do not see xml configuration of your check , and only see HTML result , please do push if smth is left on your local.

thanks,
Roman Ivanov

Roman Ivanov

unread,
Mar 4, 2015, 12:41:51 AM3/4/15
to sevntu-c...@googlegroups.com
Hi Andrew,


4)
protected String getAnnotationName(DetailAST aAnnotation)

should be static util method 

5)
>  protected String[] getAnnotationParameters(DetailAST aAnnotation)

should be Util static method of your Check

6)
> parameterstList.add(i);
> names[count++] = itemIter.getFirstChild().getText();

no need to collect AST first and then process that collection to get another collection of text, create required collection at ones and please return a List type, staying at Array do not give any benefit there.

============================


7) 
please remove original author and put your name, as looks like you will rewrite whole Check.

8) 
> Example: If you set the values "firsrtParameter" in requiredParameters and "myAnnotation"

please show how configuration will look like , instead of describing that in words.

9) 
> {@literal @}myAnnotation()

please put "// violation" on each line in example to clearly indicate location of violation
please use "// correct" as trailing comment instead of above "//This is the correct code".

10) 
For example if
* you enforce the parameter "description" on @EJB, you are still free to use
* beanName if you like.

Please move examples to the bottom and do that with code example. In first paragraph we need to explain all by words only.

11) 
value="ThePropertyName,ThePropertyName2,...ThePropertyNameN"/&gt; &lt;/module&gt;

all examples of config should follow with java code example , please redo that example.

12) 
public static final String MSG_KEY = "annotation.missing.required.parameter";
private String annotationName;

Javadoc is missed.

13) 
This will be using a LinkedHashSet

please update javadoc , the same in all other places .

14)
private String annotationName;

please make a UTs  where annotation is written by canonical name (with package). Check should fail to recognize that annotation.
Please update a Check option to ask user to provide you canonical name of type to let you recognize short and long variant of name.

15) 
please override getRequiredTokens() in this Check, in new Checkstyle it is fully abstract method.

16) 
List<String> missingParameters = new ArrayList<String>(
requiredParameters);
String parameterNames[] = getAnnotationParameters(aAST);

I do not like that attempt to copy of collection. Please reimplement methos  getAnnotationParameters to receive array of required parameters and return missed.


thanks,
Roman Ivanov

Roman Ivanov

unread,
Mar 6, 2015, 6:12:45 PM3/6/15
to sevntu-c...@googlegroups.com
Hi Andrew,


thanks,
Roman Ivanov

Roman Ivanov

unread,
Mar 6, 2015, 6:13:34 PM3/6/15
to sevntu-c...@googlegroups.com
Hi Andrew,

just fyi: on how to use checkstyle-tester project

thanks,
Roman Ivanov

Andrew Uljanenko

unread,
Mar 11, 2015, 2:48:15 PM3/11/15
to sevntu-c...@googlegroups.com

Clebert Suconic

unread,
Mar 11, 2015, 10:56:47 PM3/11/15
to sevntu-c...@googlegroups.com
thanks for taking over this.. I'm really busy on the activemq-6 land.
and I will use this on activemq6 as soon as it's released.


I know mine was just a small contribution, and you refactored a lot,
but I would appreciate a little mention. either an @author or a
mention at the commit message :)


If you don't think it's worth it and that the class was totally
rewritten I'm fine..

Roman Ivanov

unread,
Mar 12, 2015, 1:46:31 AM3/12/15
to sevntu-c...@googlegroups.com
Hi Clebert Suconic,

It was me who advised Andrew to remove you from authors as check was seriously rewritten and still require additional work, 
but we will keep you as Author or check idea https://github.com/sevntu-checkstyle/sevntu.checkstyle/issues/321
commit message will reference to issue.

thanks,
Roman Ivanov

Roman Ivanov

unread,
Mar 12, 2015, 2:13:01 AM3/12/15
to sevntu-c...@googlegroups.com
Hi Andrew,

please do "Done" for each point, even there a lot of them.


1) 
> Say you want to

please avoid any "you" in documentation, description should very formal

2)
> The name of the annotation where we are enforcing the parameter.

The name of the target annotation where we are enforcing the parameter.

3)
> Set of parameters that are required on the configured annotation

Set of parameter names that are required on the target annotation

4)
>  * They can be specified on any order and you may use additional parameters, but these
* parameters need to be filled.

 * Names can be specified on any order in target annotation

5)
&lt;module name="RequiredParameterForAnnotation"&gt; &lt;property name="annotationName"

In all XML examples please  ass "\n" after each &gt;

6)
> someMethod() {
* }

please make them as someMethod() {}
and make example with 3 parameters 

7)
> TheAnnotation(ThePropertyName1=1, ThePropertyName2=2, ThePropertyName3=3) //Correct.

make it as to show that Chck do not validate order:
TheAnnotation(ThePropertyName3=3, ThePropertyName2=2, ThePropertyName1=1) //Correct.

8)
if (requiredPropertiesParameter != null) {

do you have UT that put null in that setter ?
If that is not possible please remove that Check.

9) 
DetailAST dotNode = aAnnotationNode.findFirstToken(TokenTypes.DOT);

please move calculation as close to usage as possible.

10)
> private List<String> getAnnotationParameters(DetailAST aAnnotation)

should it be static ? 
please move it to the bottom of class, methods should appear in class according to their usage in class.

11) 
ArrayList<String> parametersList

List<String> parametersList

12) 
private List<String> getMissingAnnotationParameters(DetailAST aAnnotation)

can we use some method from Guava that do intersection of collections ?

thanks,
Roman Ivanov

Clebert Suconic

unread,
Mar 12, 2015, 2:18:56 AM3/12/15
to sevntu-c...@googlegroups.com
a commit message is fine.

I'm currently using my old version at activemq6, and I will make a
reference to this whenever is released.


Thank you

Andrew Uljanenko

unread,
Mar 12, 2015, 6:15:02 PM3/12/15
to sevntu-c...@googlegroups.com
Hi Roman,

1 I deleted this sentence
2 Done
3 Done
4 Done
5 Done
6 Done
7 Done
8 Deleted 
9 Done
10 Done
11 Done
12 Done

Roman Ivanov

unread,
Mar 13, 2015, 12:43:02 AM3/13/15
to sevntu-c...@googlegroups.com
Hi Andrew,


1)
> * Marks a given parameter as required for a given annotation. This only certify
* the minimal parameters that need to be used on the annotation.

Check that annotation is used with all required parameters.

2) 
> The name of the target annotation where we are enforcing the
* parameter.

The name of the target annotation where enforcement of parameter should happen

3) 
Please update other files with latest version of that Javadoc.

4) 
Set<String> parametersList
 

List ?

5) please recheck that no exceptions are introduced ( by checkstyle-tester) after our latest re-factoring.

6) Please do PR (pull request).

thanks,
Roman Ivanov

Andrew Uljanenko

unread,
Mar 13, 2015, 3:34:05 PM3/13/15
to sevntu-c...@googlegroups.com
Hi Roman,
1 Done
2 Done
3 Done
4 Changed
5 Tester has not revealed an exceptions
6 Done. Pull request

Clebert Suconic

unread,
May 24, 2017, 5:54:15 PM5/24/17
to Sevntu Checkstyle, andrew.u...@gmail.com
Any chance @Roman you could deploy sevntu on maven central? it's quite easy to setup an account and do it.


the repo we're using keeping going out... We are using this on activeMQ artemis and I would hate to let it go for such simple problem.

Clebert Suconic

unread,
May 24, 2017, 5:55:13 PM5/24/17
to Sevntu Checkstyle, andrew.u...@gmail.com
Really old issue here: sevntu-checkstyle/sevntu.checkstyle#165


Any chance this could be done?

Roman Ivanov

unread,
May 24, 2017, 7:21:52 PM5/24/17
to sevntu-c...@googlegroups.com, Andrew Uljanenko
release process is a bit automated
I need help from community to make sure it continue to work and deploy to maven central additionally (as not all jars that we generate could be stored in maven).

Issue is not done only because I over busy in main project with active Pull Requests (PR) and feedback to active contributors.
If smb could help this issue to be done - I will be happy to accept PR.


--
You received this message because you are subscribed to the Google Groups "Sevntu Checkstyle" group.
To unsubscribe from this group and stop receiving emails from it, send an email to sevntu-checkstyle+unsubscribe@googlegroups.com.
Reply all
Reply to author
Forward
0 new messages