Add support to ignore JSNI methods for Google style config

[Zopsss]

Add support to ignore JSNI methods for Google style config

[nrmancuso]

We need to create new "it" tests in this chapter, similar to what was completed in https://github.com/checkstyle/checkstyle/pull/4149/files.

[nrmancuso]

@Zopsss I have updated the issue description to reflect what we expect to be done here, please send a PR

[Zopsss]

Me and @romani discussed this before in gitter chat. The discussion was focused on Google Java Style Guide, in that we needed to ignore the length of JSNI methods. He suggested me to use the suppression but I guess my way will be more convenient. Here is the thread link to our conversation: link.I would be happy to hear what other maintainer's think about how we should approach this.

@rnveach @nrmancuso

[romani]

Let's restate issue, from Add support to ignore JSNI methods for LineLength check to Add support to ignore JSNI methods for Google style config

CLI execution should show usage of Google config file. Expected no violations.
We might resolve it by update of Check by usage of filter in config.
Filter is preferable, as line length module doesn't operate on AST.

[nrmancuso]

@Zopsss please link to where this is described in the Google Style guide.

[Zopsss]

@Zopsss please link to where this is described in the Google Style guide.

Here it is:

Lines where obeying the column limit is not possible (for example, a long URL in Javadoc, or a long JSNI method reference).

4.4 Column Limit: 100

Checkstyle google checks doc:

JSNI could not be detected right now, but might be possible after comments and javadoc support appear in Checkstyle.

https://checkstyle.org/google_style.html#a4.4

[nrmancuso]

These details should be in the issue description, please place them there.

This looks like it is solvable by https://checkstyle.org/filters/suppresswithplaintextcommentfilter.html#SuppressWithPlainTextCommentFilter

Inspired by my example at #11867 (comment):

➜  src cat Test.java                                             
public class Test {
  public static native void alertMessage(String msg, int a, int b, int c, int d, int e, int f, int g, int h) /*-{
    $wnd.alert(msg);
    console.log('a really long message here blah blah blah bruh bruh bruhhhhhhhhhhhhhhhhhhhhhhhhhhhh');
  }-*/;

  // violation below, we do not have JSNI delimiters here
  public static final String s = "rrrrrrrrrrrrrrrrrrrrrrrsssssssssssssssssssssssssssssssssssssssssssssssssstr";
}
➜  src javac Test.java                                             
➜  src cat config.xml                                              
<?xml version="1.0"?>
<!DOCTYPE module PUBLIC
        "-//Checkstyle//DTD Checkstyle Configuration 1.3//EN"
        "https://checkstyle.org/dtds/configuration_1_3.dtd">

<module name="Checker">
    <module name="LineLength">
      <property name="max" value="100"/>
      <property name="id" value="line-length-check"/>
    </module>
    <module name="SuppressWithPlainTextCommentFilter">
        <property name="offCommentFormat" value='\/\*-\{'/>
        <property name="onCommentFormat" value='\}-\*/'/>
    </module>
</module>%                                                                                  

➜  src java -jar checkstyle-10.13.0-all.jar -c config.xml Test.java 
Starting audit...
[ERROR] /home/nick/IdeaProjects/tester/src/Test.java:8: Line is longer than 100 characters (found 111). [line-length-check]
Audit done.
Checkstyle ends with 1 errors.

[Zopsss]

These details should be in the issue description, please place them there.

Done

This looks like it is solvable by https://checkstyle.org/filters/suppresswithplaintextcommentfilter.html#SuppressWithPlainTextCommentFilter

Yeah this can be solved using this filter but I was thinking if maybe some user wants to ignore JSNI method's length then he need to compulsory use the filter, there is no property to set to ignore the JSNI methods. If there is some property available then it will be less overhead for user to achieve this, that's my point.

[nrmancuso]

We won’t be adding a new property for something like this; new properties add complexity and maintenance overhead. Issue is easily solved by filter. Please update the issue description to have the premise described at #14487 (comment). We would only be adding this filter to google_checks.xml.

[Zopsss]

We won’t be adding a new property for something like this; new properties add complexity and maintenance overhead. Issue is easily solved by filter. Please update the issue description to have the premise described at #14487 (comment). We would only be adding this filter to google_checks.xml.

Okayy got it! I'll do it once I get back on my laptop.

[Zopsss]

We won’t be adding a new property for something like this; new properties add complexity and maintenance overhead. Issue is easily solved by filter. Please update the issue description to have the premise described at #14487 (comment). We would only be adding this filter to google_checks.xml.

sorry for the late reply, I have updated the issue description as you both suggested. @romani @nrmancuso can you check that out pls?

[nrmancuso]

Let's make sure we have an agreement on the scope of the work here, I am not 100% familiar with all the nuances of our google style guide coverage. I think we need to:

1. Update documentation + style guide (specifically mentions this case)
2. Write google style related test, this goes in it folder, in this specific chapter
3. Add module to config
   • with a comment about why it is there
   • probably placed near the linelength module?

@Zopsss can you find some other PRs with some updates like this and see what was done there? @romani anything else you can think of to add here?

[Zopsss]

Let's make sure we have an agreement on the scope of the work here, I am not 100% familiar with all the nuances of our google style guide coverage. I think we need to:

1. Update documentation

2. Write google style related test?

3. Add module to config

   • with a comment about why it is there
   • probably placed near the linelength module?

4. ...

Yeah, we will need to update the documentation, adding examples of JSNI methods to indicate that we now also support ignoring JSNI methods, we will need to write a test case to ensure that this functionality will be consistent in future and of course we will need to add suppression to our google config near LineLength check. I agree with all your points 👍

@Zopsss can you find some other PRs with some updates like this and see what was done there? @romani anything else you can think of to add here?

okay I will try to find it and will update you as soon as possible.

[romani]

Yes, plus:
Update xdoc file for style guide to remove comment that we are not covering it.

[Zopsss]

Yes, plus: Update xdoc file for style guide to remove comment that we are not covering it.

okay I will do it once I start working on this issue.

[Zopsss]

@Zopsss can you find some other PRs with some updates like this and see what was done there? @romani anything else you can think of to add here?

@nrmancuso I found this PR in which support for validating import and package statement for LineLength check were added. The contributor has updated the check itself to add support and also made test cases to ensure the new functionality is working fine, he updated the doc based on the new functionality and also updated the google check config respectively. This is similar to our case but we don't need to change the check implementation and it's documentation as we're only adding suppression in our google_checks.xml, we will only need to update the 4.4 Column Limit: 100 section of Checkstyle's google java style coverage documentation and write a google style test to ensure everything is working fine. And of course we need to add the suppression near LineLength check and write a comment explaining why it is there.

[Zopsss]

I guess now we have a clear goal to work on. Can I start working on this issue? @romani

[Zopsss]

We need to create new "it" tests in this chapter, similar to what was completed in https://github.com/checkstyle/checkstyle/pull/4149/files.

Okay but I have a doubt.... Why do we write some test cases in it folder? What is the use of that folder? We have a test folder to write our test cases then why do we need an extra folder for testing? I have seen that it contains only certain type of tests, mostly Xpath regression tests but I still couldn't understand the use of it. I had this doubt for a long time but didn't find any answer for that. @nrmancuso

[romani]

Why do we write some test cases in it folder?

to be sure that jav code written in Input file has violation or has no violation, and does not matter how much Checks/filters make it possible.
We have general Checks that can be used in any style config, in Google we configure them in specific for Google way, so we test not Check/Filter we test whole config to comply to styleguide rules.

[Zopsss]

Why do we write some test cases in it folder?

to be sure that jav code written in Input file has violation or has no violation, and does not matter how much Checks/filters make it possible. We have general Checks that can be used in any style config, in Google we configure them in specific for Google way, so we test not Check/Filter we test whole config to comply to styleguide rules.

sorry but can you explain it in more detail? I'm still not able to understand it properly

[romani]

https://github.com/checkstyle/checkstyle/tree/master/src/test this is testing of each module (checks, filters ) completely independently. Inputs covers all possible permutations of Check properties.

https://github.com/checkstyle/checkstyle/tree/master/src/it/java/com this is testing of config file ( combination of multiple Checks/filters), for now we have 2 config ( Google and Sun). So Inputs are good and not good for Google style.

[Zopsss]

https://github.com/checkstyle/checkstyle/tree/master/src/test this is testing of each module (checks, filters ) completely independently. Inputs covers all possible permutations of Check properties.

https://github.com/checkstyle/checkstyle/tree/master/src/it/java/com this is testing of config file ( combination of multiple Checks/filters), for now we have 2 config ( Google and Sun). So Inputs are good and not good for Google style.

okay and what is the use of https://github.com/checkstyle/checkstyle/tree/master/src/it/java/org/checkstyle ? Specifically this directory: https://github.com/checkstyle/checkstyle/tree/master/src/it/java/org/checkstyle/suppressionxpathfilter

I wanted to know the reason behind using the it folder for testing some specific tests

[romani]

it means "integration tests", it is not a unit test, integration means combination of multiple. In this case it is combination of Check+XpathFilter. In this tests we are testing modules together.

[Zopsss]

it means "integration tests", it is not a unit test, integration means combination of multiple. In this case it is combination of Check+XpathFilter. In this filter we are testing modules together.

got it! thanks for the explanation :)