Issue #3440: Added new check for verifying that annotation located on same line with its target #4895

kazachka · 2017-08-03T20:41:48Z

Added new check for verifying that annotation located on same line with its target.

codecov-io · 2017-08-03T21:26:34Z

Codecov Report

Merging #4895 into master will not change coverage.
The diff coverage is 100%.

@@          Coverage Diff           @@
##           master   #4895   +/-   ##
======================================
  Coverage     100%    100%           
======================================
  Files         293     294    +1     
  Lines       15897   15925   +28     
  Branches     3606    3614    +8     
======================================
+ Hits        15897   15925   +28

Impacted Files	Coverage Δ
...e/checks/annotation/AnnotationOnSameLineCheck.java	`100% <100%> (ø)`
...pycrawl/tools/checkstyle/PackageObjectFactory.java	`100% <100%> (ø)`	⬆️

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update e51f945...91144e4. Read the comment docs.

romani

please add your Check to
https://github.com/checkstyle/checkstyle/blob/master/pom.xml#L1633 (shippable.yml use such profiles)
and make sure mutation level is 100% for your Check.

items to improve:

romani · 2017-08-04T03:17:35Z

src/main/resources/com/puppycrawl/tools/checkstyle/checks/annotation/messages.properties

 annotation.trailing.comma.missing=Annotation array values must contain trailing comma.
 annotation.trailing.comma.present=Annotation array values cannot contain trailing comma.
 javadoc.duplicateTag=Duplicate {0} tag.
 javadoc.missing=Missing a Javadoc comment.
 suppressed.warning.not.allowed=The warning ''{0}'' cannot be suppressed at this location.
 tag.not.valid.on=The Javadoc {0} tag is not valid at this location.
-


please avoid non required changes in such big update to code. In other files too.

deleted
now there is different empty lines in different language files

romani · 2017-08-04T03:18:33Z

src/xdocs/config_annotation.xml

@@ -228,6 +228,111 @@ public void test(&#64;MyAnnotation String s) { // OK
      </subsection>
    </section>

+    <section name="AnnotationOnSameLine">
+      <subsection name="Description">
+        <p>Since Checkstyle 8.1</p>


8.2 as minimum

romani · 2017-08-04T03:21:56Z

.../tools/checkstyle/checks/annotation/annotationonsameline/InputAnnotationOnSameLineCheck.java

+
+    @Annotation
+    @SomeClass.Annotation
+    public int getX() {


please provide more tests, 2 test cases if not enough.
Please reuse all cases that are mentioned in issue. Please provide more tests for all tokens(methods, fields, classes, interfaces, ....).

added cases from issue and for all of tokens except of DOT (i can't find any usage of annotation with this token)

rnveach · 2017-08-04T19:32:30Z

...st/java/com/puppycrawl/tools/checkstyle/checks/annotation/AnnotationOnSameLineCheckTest.java

+        final DefaultConfiguration config = createModuleConfig(AnnotationOnSameLineCheck.class);
+        final String[] expected = {
+            "9: " + getCheckMessage(MSG_KEY_ANNOTATION_ON_SAME_LINE, "Annotation"),
+            "10: " + getCheckMessage(MSG_KEY_ANNOTATION_ON_SAME_LINE, "SomeClass"),


Isn't annotation name Annotation and not SomeClass?
If I wrote out java.lang.Override, annotation name isn't java, it is Override.
Please add java.lang.Override as an example too.

Method updated.
Note that i copy it from AnnotationLocationCheck, so AnnotationLocation can wrote "SomeClass" instead "Annotation".
Added java.lang.Deprecated as example.

rnveach · 2017-08-04T19:33:41Z

src/xdocs/config_annotation.xml

+              <a href="apidocs/com/puppycrawl/tools/checkstyle/api/TokenTypes.html#CTOR_DEF">CTOR_DEF</a>,
+              <a href="apidocs/com/puppycrawl/tools/checkstyle/api/TokenTypes.html#VARIABLE_DEF">VARIABLE_DEF</a>.
+            </td>
+            <td>8.1</td>


should be 8.2 too.

rnveach · 2017-08-13T12:13:15Z

@kazachka Shippable failed.

-Ppitest-checks-annotation
[ERROR] Failed to execute goal org.pitest:pitest-maven:1.2.2:mutationCoverage (default-cli) on project checkstyle: Mutation score of 93 is below threshold of 100 -> [Help 1]

romani · 2017-08-17T14:06:01Z

@kazachka , please make sure that there is 100% coverage on your Check, in pitest report.

kazachka · 2017-08-18T17:48:11Z

I just forgot to add test to pitest configuration in pom.xml

rnveach · 2017-08-18T19:09:33Z

pom.xml

@@ -1675,6 +1675,7 @@
            <configuration>
              <targetClasses>
                <param>com.puppycrawl.tools.checkstyle.checks.annotation.AnnotationLocationCheck</param>


@romani Why wasn't this com.puppycrawl.tools.checkstyle.checks.annotation.*?

because of hcoles/pitest#368

romani · 2017-08-24T14:24:04Z

@kazachka , please provide testing report of this Check over multiple projects, please use our checkstyle-tester tool

romani

Last item to improve.

@rnveach , please finish your review

romani · 2017-08-24T14:30:17Z

src/xdocs/config_annotation.xml

+      <subsection name="Description">
+        <p>Since Checkstyle 8.2</p>
+        <p>
+          Check annotation located on same line with its target.


We need make a note that this Check is not a good practice, but some style guides use it.

Make sure this note is in the check's Javadoc.

Note added to both xdocs and javadoc

rnveach

I don't see anything else right now.

rnveach · 2017-08-27T13:52:53Z

src/main/java/com/puppycrawl/tools/checkstyle/checks/annotation/AnnotationOnSameLineCheck.java

+     * @return node that is next to given
+     */
+    private static DetailAST getNextNode(DetailAST node) {
+        final DetailAST nextNode;


All your other forms of writing this was writing the assignment with the standard one, and than change it's assignment if it was null. Why did you change it here?

DetailAST nextNode = node.getNextSibling(); if (nextNode == null) { nextNode = node.getParent().getNextSibling(); }

rnveach · 2017-08-27T13:55:40Z

src/xdocs/checks.xml

@@ -48,6 +48,10 @@
        <td>Check location of annotation on language elements.</td>
      </tr>
      <tr>
+        <td><a href="config_annotation.html#AnnotationOnSameLine">AnnotationOnSameLine</a></td>
+        <td>Check annotation on language elements is on same line with its target element.</td>


Why isn't this the same text as in the check in config_annotation or in the main Javadoc for the check?

Updated to javadoc text

rnveach · 2017-08-27T13:56:28Z

src/xdocs/config_annotation.xml

+      <subsection name="Description">
+        <p>Since Checkstyle 8.2</p>
+        <p>
+          Check annotation located on same line with its target.


Please make this description the same the check's javadoc.

Updated to javadoc text

…located on the same line with its target

kazachka · 2017-08-27T21:12:13Z

Diff report:
https://kazachka.github.io/single-line-annotation-diff-report/

romani · 2017-08-28T19:54:16Z

As soon as TC passed , we could merge. but lets wait for TC to finish.

kazachka force-pushed the single-line-annotation-check branch from 2cd4673 to ce7526a Compare August 3, 2017 20:44

romani requested changes Aug 4, 2017

View reviewed changes

rnveach requested changes Aug 4, 2017

View reviewed changes

kazachka force-pushed the single-line-annotation-check branch from ce7526a to 44d75dd Compare August 12, 2017 19:15

kazachka force-pushed the single-line-annotation-check branch from 44d75dd to 941570c Compare August 18, 2017 16:52

rnveach reviewed Aug 18, 2017

View reviewed changes

romani requested changes Aug 24, 2017

View reviewed changes

rnveach requested changes Aug 27, 2017

View reviewed changes

Issue checkstyle#3440: Added new check for verifying that annotation …

91144e4

…located on the same line with its target

kazachka force-pushed the single-line-annotation-check branch from 941570c to 91144e4 Compare August 27, 2017 21:08

rnveach approved these changes Aug 28, 2017

View reviewed changes

romani approved these changes Aug 28, 2017

View reviewed changes

romani merged commit 76b6f99 into checkstyle:master Aug 28, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Issue #3440: Added new check for verifying that annotation located on same line with its target #4895

Issue #3440: Added new check for verifying that annotation located on same line with its target #4895

kazachka commented Aug 3, 2017

codecov-io commented Aug 3, 2017 •

edited

romani left a comment

romani Aug 4, 2017

kazachka Aug 12, 2017

romani Aug 4, 2017

kazachka Aug 12, 2017

romani Aug 4, 2017

kazachka Aug 12, 2017

rnveach Aug 4, 2017

kazachka Aug 12, 2017

rnveach Aug 4, 2017

kazachka Aug 12, 2017

rnveach commented Aug 13, 2017

romani commented Aug 17, 2017

kazachka commented Aug 18, 2017

rnveach Aug 18, 2017

romani Aug 18, 2017

romani commented Aug 24, 2017

romani left a comment

romani Aug 24, 2017

rnveach Aug 27, 2017

kazachka Aug 27, 2017

rnveach left a comment

rnveach Aug 27, 2017

kazachka Aug 27, 2017

rnveach Aug 27, 2017

kazachka Aug 27, 2017

rnveach Aug 27, 2017

kazachka Aug 27, 2017

kazachka commented Aug 27, 2017

romani commented Aug 28, 2017

Issue #3440: Added new check for verifying that annotation located on same line with its target #4895

Issue #3440: Added new check for verifying that annotation located on same line with its target #4895

Conversation

kazachka commented Aug 3, 2017

codecov-io commented Aug 3, 2017 • edited

Codecov Report

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

rnveach commented Aug 13, 2017

romani commented Aug 17, 2017

kazachka commented Aug 18, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

romani commented Aug 24, 2017

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

rnveach 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

kazachka commented Aug 27, 2017

romani commented Aug 28, 2017

codecov-io commented Aug 3, 2017 •

edited