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

new Check: single line annotation location #3440

Closed
vanniktech opened this Issue Sep 12, 2016 · 6 comments

Comments

Projects
None yet
4 participants
@vanniktech

vanniktech commented Sep 12, 2016

I'd love to have AnnotationLocation extended in a way that the following code would fail:

@NonNull
private List<String> names = new ArrayList<>();

Since it should be

@NonNull private List<String> names = new ArrayList<>();

Until version 7.1.1 this is not possible. You can only white list the latter sample but not tell the check to fail on the first example.

update:
AnnotationLocation can not be updated as it have 3 properties that allowSamelineXXXXXX . So update have new option that will demand singleline will looks weird in Checkdesign. So new check have to be created.

@romani romani changed the title from Extend AnnotationLocation to new Check: single line annotation location Sep 12, 2016

@romani romani added the approved label Sep 12, 2016

@rnveach rnveach added the new module label Mar 2, 2017

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Jul 15, 2017

Member

@vanniktech ,

Do you expect violations on annotation locations like:
1)

@SuppressWarnings("deprecation")
@Mock DataLoader loader;
@SuppressWarnings("deprecation")
@Mock
DataLoader loader;

and NO violations on code like:
3)

@SuppressWarnings("deprecation") @Mock @Mock2 @Mock3 @Mock4 DataLoader loader;

please provide more cases with code and notes where you expect violations.
As forcing all annotations to be on single line is not always practical. From my perspective only single non-parameters annotation is ok to keep on the same line.

Member

romani commented Jul 15, 2017

@vanniktech ,

Do you expect violations on annotation locations like:
1)

@SuppressWarnings("deprecation")
@Mock DataLoader loader;
@SuppressWarnings("deprecation")
@Mock
DataLoader loader;

and NO violations on code like:
3)

@SuppressWarnings("deprecation") @Mock @Mock2 @Mock3 @Mock4 DataLoader loader;

please provide more cases with code and notes where you expect violations.
As forcing all annotations to be on single line is not always practical. From my perspective only single non-parameters annotation is ok to keep on the same line.

@vanniktech

This comment has been minimized.

Show comment
Hide comment
@vanniktech

vanniktech Jul 17, 2017

First test case

Input

@SuppressWarnings("deprecation")
@Mock DataLoader loader;

Output

1 violation since I'd like to be like this:

@SuppressWarnings("deprecation") @Mock DataLoader loader;

Second

Input

@SuppressWarnings("deprecation")
@Mock
DataLoader loader;

Output

2 violations (one per annotation) since I'd like to be like this:

```java
@SuppressWarnings("deprecation") @Mock DataLoader loader;

Third

Input

@SuppressWarnings("deprecation") @Mock @Mock2 @Mock3 @Mock4 DataLoader loader;

Output

0 violations since all annotations are aligned properly.


As forcing all annotations to be on single line is not always practical. From my perspective only single non-parameters annotation is ok to keep on the same line.

Maybe it's possible to use the MaxLineLength property for this and allow it to break if the limit is reached? In 99% of the cases they don't exceed the line limit. Even @SuppressWarnings("deprecation") @Mock @Mock2 @Mock3 @Mock4 DataLoader loader; is still fine since it has 78 characters.

I hope this is clear enough.

vanniktech commented Jul 17, 2017

First test case

Input

@SuppressWarnings("deprecation")
@Mock DataLoader loader;

Output

1 violation since I'd like to be like this:

@SuppressWarnings("deprecation") @Mock DataLoader loader;

Second

Input

@SuppressWarnings("deprecation")
@Mock
DataLoader loader;

Output

2 violations (one per annotation) since I'd like to be like this:

```java
@SuppressWarnings("deprecation") @Mock DataLoader loader;

Third

Input

@SuppressWarnings("deprecation") @Mock @Mock2 @Mock3 @Mock4 DataLoader loader;

Output

0 violations since all annotations are aligned properly.


As forcing all annotations to be on single line is not always practical. From my perspective only single non-parameters annotation is ok to keep on the same line.

Maybe it's possible to use the MaxLineLength property for this and allow it to break if the limit is reached? In 99% of the cases they don't exceed the line limit. Even @SuppressWarnings("deprecation") @Mock @Mock2 @Mock3 @Mock4 DataLoader loader; is still fine since it has 78 characters.

I hope this is clear enough.

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Jul 17, 2017

Member

Maybe it's possible to use the MaxLineLength property for this and allow it to break if the limit is reached?

we can not use other Check settings, all Checks are independent. You will have to play with suppression if you want to avoid conflicts.

Member

romani commented Jul 17, 2017

Maybe it's possible to use the MaxLineLength property for this and allow it to break if the limit is reached?

we can not use other Check settings, all Checks are independent. You will have to play with suppression if you want to avoid conflicts.

@vanniktech

This comment has been minimized.

Show comment
Hide comment
@vanniktech

vanniktech Jul 17, 2017

Then make it a property of this new check and give it the same default value?

vanniktech commented Jul 17, 2017

Then make it a property of this new check and give it the same default value?

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Jul 17, 2017

Member

no, it is sign of bad design or bad idea of Check.
If user come to extreme code that is still valid from his perspective - he need to use suppressions.

Member

romani commented Jul 17, 2017

no, it is sign of bad design or bad idea of Check.
If user come to extreme code that is still valid from his perspective - he need to use suppressions.

kazachka added a commit to kazachka/checkstyle that referenced this issue Aug 3, 2017

kazachka added a commit to kazachka/checkstyle that referenced this issue Aug 3, 2017

kazachka added a commit to kazachka/checkstyle that referenced this issue Aug 12, 2017

kazachka added a commit to kazachka/checkstyle that referenced this issue Aug 18, 2017

kazachka added a commit to kazachka/checkstyle that referenced this issue Aug 27, 2017

romani added a commit that referenced this issue Aug 28, 2017

@romani romani added the new feature label Aug 28, 2017

@romani romani added this to the 8.2 milestone Aug 28, 2017

@romani

This comment has been minimized.

Show comment
Hide comment
@romani

romani Aug 28, 2017

Member

fix is merged.

Member

romani commented Aug 28, 2017

fix is merged.

@romani romani closed this Aug 28, 2017

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment