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

Specify violation messages in input files #11214

Closed
50 of 55 tasks
Vyom-Yadav opened this issue Jan 18, 2022 · 115 comments · Fixed by #11321 or #14000
Closed
50 of 55 tasks

Specify violation messages in input files #11214

Vyom-Yadav opened this issue Jan 18, 2022 · 115 comments · Fixed by #11321 or #14000

Comments

@Vyom-Yadav
Copy link
Member

Vyom-Yadav commented Jan 18, 2022

Single Check might have multiple violations messages, we need to see exact violation message if Check has multiple messages, to be sure that violation of correct type is placed at right line.

Violation messages have not been specified in certain input files, a list of checks in which violation message is not specified in the input file and have more than one violation message key has been specified here-

private static final Set<String> SUPPRESSED_CHECKS = new HashSet<>(Arrays.asList(

To Do-

  • Pick a check (single Check Inputs files update in single Pull Request) To allocate Check update for you please make comment in issue "I am on XxxxxCheck"
  • Find all input files that use that check
  • Add violation message at the end of the // violation comment
  • Remove the check from the suppression list

Same has been demonstrated at 461cd04
If the violation message contains special regex characters, escape that character.
Ignore input files using more than a single check.

Ignore lines with multiple violations, eg-

// 2 violations
// 2 violations above
// 4 violations below

and any other violation message of this nature.

List of Checks:

  • RegexpCheck
  • EmptyForInitializerPadCheck
  • JavadocStyleCheck
  • SummaryJavadocCheck

Old tracker (some might be outdated):

  • RightCurlyCheck
  • EqualsAvoidNullCheck
  • MethodParamPadCheck
  • DescendantTokenCheck
  • RequireThisCheck
  • EqualsHashCodeCheck
  • JavaNCSSCheck
  • ReturnCountCheck
  • AnnotationUseStyleCheck
  • EmptyLineSeparatorCheck
  • OrderedPropertiesCheck
  • AbstractJavadocCheck
  • DeclarationOrderCheck
  • MissingOverrideCheck
  • AbstractClassNameCheck
  • MethodCountCheck
  • PackageDeclarationCheck
  • JavadocTypeCheck
  • LeftCurlyCheck
  • RegexpCheck
  • WhitespaceAfterCheck
  • MultipleVariableDeclarationsCheck
  • EmptyForInitializerPadCheck
  • UnnecessaryParenthesesCheck
  • JavadocMethodCheck
  • ModifierOrderCheck
  • MissingDeprecatedCheck
  • RegexpOnFilenameCheck
  • RedundantImportCheck
  • EmptyBlockCheck
  • UniquePropertiesCheck
  • WhitespaceAroundCheck
  • OperatorWrapCheck
  • GenericWhitespaceCheck
  • EmptyForIteratorPadCheck
  • JavadocStyleCheck
  • JavadocPackageCheck
  • FallThroughCheck
  • WriteTagCheck
  • TranslationCheck
  • FileTabCharacterCheck
  • ImportOrderCheck
  • JavadocContentLocationCheck
  • JavadocParagraphCheck
  • SummaryJavadocCheck
  • DefaultComesLastCheck
  • AnnotationLocationCheck
  • CustomImportOrderCheck
  • NewlineAtEndOfFileCheck
  • VariableDeclarationUsageDistanceCheck
  • ImportControlCheck

format it like:

public static final int FOO = 3; // violation 'Variable access definition in wrong order.'

if message is long, we can shorten it by regexp usage (but do not over use regexp, message should be readable):

There are a lot of examples in code just search for // violation in Input files for more examples.

@Kevin222004
Copy link
Collaborator

I am on WhitespaceAfterCheck

@SarveshLimaye
Copy link
Contributor

SarveshLimaye commented Feb 16, 2022

I am on EmptyBlockCheck

@Kevin222004
Copy link
Collaborator

I am on WhitespaceAfterCheck
@Vyom-Yadav I have found 13 input files contain the message // violation. can I sharing a image to know that i am on right path or not

@Vyom-Yadav
Copy link
Member Author

@Kevin222004 Sure

@Vyom-Yadav
Copy link
Member Author

@Kevin222004 currently we have // violation comment, you have to replace it with the actual violation message, you can see the violation message from respected test file, you can use 461cd04 as reference.

@SarveshLimaye
Copy link
Contributor

@Vyom-Yadav I am not able to find actual violation msg. Can you pls help me with it ?

@Kevin222004
Copy link
Collaborator

Kevin222004 commented Feb 17, 2022

@Kevin222004 currently we have // violation comment, you have to replace it with the actual violation message, you can see the violation message from respected test file, you can use 461cd04 as reference.

@Vyom-Yadav can I go through this documentation
https://checkstyle.sourceforge.io/config_whitespace.html#WhitespaceAfter

@Vyom-Yadav
Copy link
Member Author

@SarveshLimaye It will be in the .properties file, if you are not able to find it, just search within the whole project (Ctrl + Shift + F in intellij).

@Vyom-Yadav can I go through this documentation

Yes, you can read whatever you wish to.

@SarveshLimaye
Copy link
Contributor

@Vyom-Yadav
Copy link
Member Author

Kindly make PR's so we can see better, every check has a violation message MSG_{someMsg} which is equal to something like block.empty or some other short key that finally points to the complete message.

MSG_KEY -> msg.key -> This is the message key.
<-------------------> 
          <------------------------------------>
Check                  .properties file

The overlapping portion is present in both the check and the .properties file.
Ctrl + click will take you to the declaration in intellij.

@Kevin222004
Copy link
Collaborator

@Vyom-Yadav How to add violation message which contain more than one violation (ex :- // 2 violation and // 3 violation )

@Vyom-Yadav
Copy link
Member Author

Vyom-Yadav commented Feb 19, 2022

@Kevin222004 You don't have to specify a violation message where there are multiple violations, you can ignore those cases, I updated PR desc to demonstrate the same.

@Kevin222004
Copy link
Collaborator

Kevin222004 commented Feb 19, 2022

@Vyom-Yadav
Screenshot from 2022-02-19 22-42-05
i am finding difficulties to solve this error

@Vyom-Yadav
Copy link
Member Author

The line shouldn't be longer than 100 characters. If your violation message is crossing 100 characters, eg-

int a = 12; // violation, some loooooooooooooooooong violation message

you can change it to-

int a = 12;
// violation above, some looooooooooooooong violation message

to reduce the line length.

@romani
Copy link
Member

romani commented Feb 19, 2022

better to keep traling comment on same line as violation
int a = 12; // violation, some lo*ng violation message

as message supports regexp and allow us to remove some obvious parts.

@Vyom-Yadav
Copy link
Member Author

@Kevin222004 It's your decision, you have to decide what to do based on some factors, first ask yourself if I shorten this message with the help of regex, will it make sense? secondly if not then try wrapping the violation part on the next line to fall within the limits, there is no set of rules, you have to decide what to do, in this particular case I would have wrapped the second argument on the next line to keep the violation message clearer.

@romani
Copy link
Member

romani commented Feb 21, 2022

@Kevin222004 , you can shorten names parameters, change used types of parameter, you can do line wrap(but it will force you do more updates in test expectation due to line number shift.).
We might do removal of first argument, as it is not required for this validations.

@SarveshLimaye
Copy link
Contributor

@Vyom-Yadav @romani I am not able to solve this errors

image

@romani
Copy link
Member

romani commented Feb 22, 2022

please send PR to let us see your problem and exact code (all details matter) that you have.

@SarveshLimaye
Copy link
Contributor

@romani have already raised pr. Could you pls take a look . #11322

@faraazb
Copy link
Contributor

faraazb commented Feb 24, 2022

Hi! I am on UnnecessaryParenthesesCheck. I used the corresponding test file to identify the input files. Will make a PR soon!

nrmancuso pushed a commit to suniti0804/checkstyle that referenced this issue Oct 29, 2023
@suniti0804
Copy link
Contributor

In the class src/test/java/com/puppycrawl/tools/checkstyle/checks/javadoc/JavadocStyleCheckTest.java it's unusual to find keyword "violation" in the expected array. For other checks that we have, we don't have this keyword in the expected array itself and only in the input files. Can I please get an understanding on this?

An example -

@Test
   public void testExcludeScope1()
           throws Exception {
       final String[] expected = {
           "23: " + getCheckMessage(MSG_NO_PERIOD),
           "48: " + getCheckMessage(MSG_NO_PERIOD),
           "56:11: " + getCheckMessage(MSG_UNCLOSED_HTML,
                   "<b>This guy is missing end of bold tag // violation"),
           "59:7: " + getCheckMessage(MSG_EXTRA_HTML, "</td>Extra tag "
                   + "shouldn't be here // violation"),
           "60:49: " + getCheckMessage(MSG_EXTRA_HTML, "</style> // violation"),
           "61:19: " + getCheckMessage(MSG_UNCLOSED_HTML, "<code>dummy. // violation"),
           "70: " + getCheckMessage(MSG_NO_PERIOD),
           "71:31: " + getCheckMessage(MSG_UNCLOSED_HTML, "<b>should fail // violation"),
           "92:39: " + getCheckMessage(MSG_EXTRA_HTML, "</img> // violation"),
       };
       verifyWithInlineConfigParser(
               getPath("InputJavadocStyleExcludeScope1.java"), expected);
   }

The expected array comes out to be as -


23: First sentence should end with a period.
48: First sentence should end with a period.
56:11: Unclosed HTML tag found: <b>This guy is missing end of bold tag // violation
59:7: Extra HTML tag found: </td>Extra tag shouldn't be here // violation
60:49: Extra HTML tag found: </style> // violation
61:19: Unclosed HTML tag found: <code>dummy. // violation
70: First sentence should end with a period.
71:31: Unclosed HTML tag found: <b>should fail // violation
92:39: Extra HTML tag found: </img> // violation

suniti0804 added a commit to suniti0804/checkstyle that referenced this issue Oct 29, 2023
suniti0804 added a commit to suniti0804/checkstyle that referenced this issue Oct 29, 2023
suniti0804 added a commit to suniti0804/checkstyle that referenced this issue Oct 29, 2023
suniti0804 added a commit to suniti0804/checkstyle that referenced this issue Oct 29, 2023
@romani
Copy link
Member

romani commented Oct 29, 2023

@suniti0804 ,

it's unusual to find keyword "violation"

Thanks a lot for raising this. It is bad design of our old tests.
We use to have this before we added support of // violation X lines below

Ideally we should refactor such tests to remove violation comment from javadoc and put them above javadoc.

@suniti0804
Copy link
Contributor

@suniti0804 ,

it's unusual to find keyword "violation"

Thanks a lot for raising this. It is bad design of our old tests. We use to have this before we added support of // violation X lines below

Ideally we should refactor such tests to remove violation comment from javadoc and put them above javadoc.

To address this, shall we make a new issue or we can make the fix here only in the issue #11214 ?

suniti0804 added a commit to suniti0804/checkstyle that referenced this issue Oct 29, 2023
suniti0804 added a commit to suniti0804/checkstyle that referenced this issue Oct 29, 2023
@romani
Copy link
Member

romani commented Oct 29, 2023

Please create new issue to update all javadoc Checks to avoid // violation in javadoc comments

@suniti0804
Copy link
Contributor

src/test/java/com/puppycrawl/tools/checkstyle/checks/javadoc/JavadocStyleCheckTest.java

We have this issue now - #13969

suniti0804 added a commit to suniti0804/checkstyle that referenced this issue Oct 30, 2023
suniti0804 added a commit to suniti0804/checkstyle that referenced this issue Oct 30, 2023
@suniti0804
Copy link
Contributor

Some of the input files using the class - FinalClassCheckTest doesn't have violation message specified in them and yet this check is not in the suppression list and evidently not causing any errors as well. What could be the reason for this?

@romani
Copy link
Member

romani commented Oct 30, 2023

Not all Input* files has violations, as some files are absolutely show how code should looks like to have no violations in it.
All methods in this Test file use verifyWithInlineConfigParser that is the best form of test we should have. Some tests without such method covers extreme edge cases.

@sushant1987
Copy link
Contributor

I am on JavadocStyleCheck

@Vyom-Yadav
Copy link
Member Author

Thanks a lot to all the contributors involved!

0xbakry pushed a commit to 0xbakry/checkstyle that referenced this issue Mar 13, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment