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

Add support to our Inputs for lines with multiple violations #13307

Closed
romani opened this issue Jun 26, 2023 · 10 comments · Fixed by #14089 or #14156
Closed

Add support to our Inputs for lines with multiple violations #13307

romani opened this issue Jun 26, 2023 · 10 comments · Fixed by #14089 or #14156

Comments

@romani
Copy link
Member

romani commented Jun 26, 2023

This was a problem for long time but become very acute at #13276 (comment)

We need to update our Abstract Test classes to allow lines like:

    Runnable noop = () ->{}; // 4 violations, see messages below 
                          // ''->' is not followed by whitespace'
                          // ''{' is not followed by whitespace'
                          // ''{' is not preceded with whitespace'
                          // ''}' is not preceded with whitespace'

so all messages are just on new lines below 4 violations, see messages below
So Reviewer will clearly see what is a violation.

example:


but there are more in code.

pattern that used at PR, may be extra : is required to signal that second line is continuation:

    /**
     *  <p> <li> <TR> <Td> <tH> <.....
     *      <tFoot> // ok
     * @param field5 </p> value to which {@link #field5} is to be set to
     */
    // 4 violations 4 lines above:
    //                            'Unclosed HTML tag found: p'
    //                            'tag P_TAG_START'
    //                            'tag LI_TAG_START'
    //                            'tag BODY_TAG_START'
    private void setField5(int field5) {this.field5 = field5;}

As we process lines one by one, of we detect multiple-line pattern we look ahead for defined set of lines. Processing of such lines with violation only will not match any pattern so they will be skipped by general iteration over lines. Location of single quoted message can be at any indentation, to let user user any formatting to show message as he wants.

@romani
Copy link
Member Author

romani commented Jun 26, 2023

@jxr98 , can you help us with this issue ?

@jxr98
Copy link
Contributor

jxr98 commented Jun 26, 2023

Thanks. I would love to help with this issue.

@romani
Copy link
Member Author

romani commented Jun 26, 2023

@jxr98 , design is not set in stone, so your suggestion to change it to get more robust implementation is appreciated.
All we need is some form of explicit and visial definition of expected violation messages.

@jxr98
Copy link
Contributor

jxr98 commented Jun 26, 2023

Gotcha. Thanks for clarifying.

@jxr98
Copy link
Contributor

jxr98 commented Sep 7, 2023

The multiple violations in one line now are specified in comments as below

for (int i = 1; i > 1; i++) {} // 2 violations
                               // 'no space after '{', no space before '}''

As Romani pointed out, we can change to the example below

for (int i = 1; i > 1; i++) {} // 2 violations, list on separate lines below
                               // 'no space after '{''
                               // 'no space before '}''

The differences between multiple violations on one line and single violation on one line:

  1. There're no numbers before the word violation in single violation. (multiple violations use the plural form)
  2. Multiple violations are followed by newlines.

We can use these two differences to distinguish multiple violations and single violation. Each line followed by the plural form(violations) will be regared as a violation, and the verifyWithInlineConfigParser function will need to check if the specified number match the number of newlines as well.
Plz leave a message if you have any idea and correct me if I'm wrong. Thanks.

@nrmancuso
Copy link
Member

nrmancuso commented Sep 9, 2023

What if we do:

    /*  4 violations below:
      ''->' is not followed by whitespace'
      ''{' is not followed by whitespace'
      ''{' is not preceded with whitespace'
      ''}' is not preceded with whitespace'
    */
    Runnable noop = () ->{};

and

    Runnable noop = () ->{};
    /*  4 violations above:
      ''->' is not followed by whitespace'
      ''{' is not followed by whitespace'
      ''{' is not preceded with whitespace'
      ''}' is not preceded with whitespace'
    */

?

I find this easier to read (less noise than single line comments and no comments on the lines with the code), and probably easier to parse since we have two ways to check for end of violation list (number of violations/lines in first comment line + block comment end delimiter).

@romani
Copy link
Member Author

romani commented Sep 9, 2023

I like both variants, if we need to choose I would use what is easier to implement now to have this feature sooner.
Author of implementation should choose a way to make it.

@jxr98
Copy link
Contributor

jxr98 commented Sep 11, 2023

From implementation perspective, I need to look into details before deciding which one is easier to implement. I will try both variants a bit.

jxr98 added a commit to jxr98/checkstyle that referenced this issue Sep 28, 2023
jxr98 added a commit to jxr98/checkstyle that referenced this issue Sep 28, 2023
jxr98 added a commit to jxr98/checkstyle that referenced this issue Oct 2, 2023
romani added a commit to romani/checkstyle that referenced this issue Dec 1, 2023
romani added a commit to romani/checkstyle that referenced this issue Dec 5, 2023
romani added a commit to romani/checkstyle that referenced this issue Dec 9, 2023
romani added a commit to romani/checkstyle that referenced this issue Dec 9, 2023
romani added a commit to romani/checkstyle that referenced this issue Dec 10, 2023
romani added a commit to romani/checkstyle that referenced this issue Dec 10, 2023
romani added a commit to romani/checkstyle that referenced this issue Dec 12, 2023
@github-actions github-actions bot added this to the 10.12.7 milestone Dec 13, 2023
@romani
Copy link
Member Author

romani commented Dec 13, 2023

Reopened for:

NOT done validation of messages for:

public class InputAbstractJavadocNonTightHtmlTags2 {
    /** <p> <p> paraception </p> </p> */
    // 3 violations above
    //                    'Unclosed HTML tag found: p'
    //                    'tag P_TAG_START'
    //                    'tag P_TAG_START'

I think it is better to do this after this PR, to make sure prove of concept is megred and second PR will be easy to accept as it will just extension.

romani added a commit to romani/checkstyle that referenced this issue Dec 20, 2023
romani added a commit to romani/checkstyle that referenced this issue Dec 21, 2023
@nrmancuso
Copy link
Member

Completed via #14156

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
3 participants