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

Suppression for long identifiers for 4.4 Column Limit: 100 rule of google java style guide #15233

Closed
Zopsss opened this issue Jul 11, 2024 · 9 comments

Comments

@Zopsss
Copy link
Contributor

Zopsss commented Jul 11, 2024

I have read check documentation: https://checkstyle.org/checks/sizes/linelength.html
I have downloaded the latest checkstyle from: https://checkstyle.org/cmdline.html#Download_and_Run
I have executed the cli and showed it below, as cli describes the problem better than 1,000 words

Detected at: #14901 (comment)

The 4.4 Column Limit: 100 rule of google java style guide has some exceptions cases in which this rule does not apply, one of them is:

Very long identifiers, on the rare occasions they are called for, are allowed to exceed the column limit. In that case, the valid wrapping for the surrounding code is as produced by google-java-format.

style guide says that long identifiers which crosses the column limit of 100 are allowed, so we should not give violation due to such identifiers.


Current behavior of Checkstyle:

package com.examples.columnlimit;

/** testing. */
public class Checkstyle {
  int aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa = 99;

  public void testtttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttt() {
    System.out.println("heloooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooo");
  }
}

Output:

$ java -jar ..\checkstyle-10.17.0-all.jar -c ..\google_checks.xml .\Checkstyle.java
Starting audit...
[WARN] \Checkstyle.java:5: Line is longer than 100 characters (found 109). [LineLength]
[WARN] \Checkstyle.java:7: Line is longer than 100 characters (found 106). [LineLength]
[WARN] \Checkstyle.java:8: Line is longer than 100 characters (found 110). [LineLength]
Audit done.

google-java-format behavior:

package com.examples.columnlimit;

/** testing. */
public class Checkstyle {
  int
      aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaa =
          99;

  public void
      testtttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttttt() {
    System.out.println(
        "heloooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooo");
  }
}

The formatter formats and tries to wrap the long identifier to make it under column limit but this still crosses the limit of 100. And in case of CS, we give violation for such type of code.


Describe what you expect in detail.

As @rdiachenko suggested here: #14901 (comment), we should revisit the working of LineLength module and add a new property to ignore long identifiers, this will also require to update the google_checks.xml accordingly.

Or

We can discuss another way to solve the problem here in this issue.

@romani
Copy link
Member

romani commented Jul 13, 2024

@Zopsss , please make PR with Input that shows shows long identifiers wrapped by formatter as he can AND how checkstyle violate them.

Long variable, method, type name, ... Give a bit more examples. Keep it formatted by formatter.
attention to example with a lot of nested code and lond identifier (20 characters) in most deep point of it. So it is not very long but still line with 100 can not fit it.

@romani romani moved this from For Later to In Progress in Refine Google Style Guide Jul 14, 2024
@romani romani moved this from In Progress to Todo in Refine Google Style Guide Jul 14, 2024
@romani
Copy link
Member

romani commented Jul 15, 2024

we should revisit the working of LineLength module and add a new property to ignore long identifiers,

it is not that easy, LineLength become not Java Check, so it does not know anything about methods and variables.

points of attention ....aaaaaaaaaaa = and ...tttttttttt() {
so tool does not break other rules for "=" and { to do line wrapping.

The only solution I see is to allow users to use suppressions to mark suppression in comment and explain reason https://checkstyle.org/google_style.html#Google_Suppressions, please play with this and lets choose what we would recomment for users to use if they run into this.

@romani romani changed the title Wrapping of long identifiers for 4.4 Column Limit: 100 rule of google java style guide Suppression for long identifiers for 4.4 Column Limit: 100 rule of google java style guide Jul 15, 2024
@Zopsss Zopsss moved this from Todo to For Later in Refine Google Style Guide Jul 17, 2024
@Zopsss
Copy link
Contributor Author

Zopsss commented Aug 2, 2024

@Zopsss , please make PR with Input that shows shows long identifiers wrapped by formatter as he can AND how checkstyle violate them.

Long variable, method, type name, ... Give a bit more examples. Keep it formatted by formatter. attention to example with a lot of nested code and lond identifier (20 characters) in most deep point of it. So it is not very long but still line with 100 can not fit it.

Umm creating a separate PR won't help much, I think it will be better to show the code here with the output, I guess this will be a more efficient way.

Original code:

/** some javadoc. */
public class TestingColumnLimit {
  public static int GLLLLLLLLLLLLLLLLLLOOOOOOOOOOOOOOOBBBBBBBBAAAAAAAAALLLLLLLLLLL_______VARRRRRRRIIIIIIIABLEEEEEEEEE = 99;

  final int abccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccc = 99;

  void fooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooo() {

    boolean xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx = true;

    if (xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx) {
      // some code
    } else {
      // some code
    }

    switch (GLLLLLLLLLLLLLLLLLLOOOOOOOOOOOOOOOBBBBBBBBAAAAAAAAALLLLLLLLLLL_______VARRRRRRRIIIIIIIABLEEEEEEEEE) {
        case abccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccc:
            fooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooo();
            break;

        default:
            break;
    }

    System.out.println("Helllllllllllllllllllllllllllllllllllllllllllllllllllllllllloooooooooooooooooooooooooooooooooo");
  }

  class Innnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnneeeeeeeeeeeeeeeeeeeeeeeerrrrrrrrrrrrrrrrrrrr {}
}

Formatted code:

/** some javadoc. */
public class TestingColumnLimit {
  public static int
      GLLLLLLLLLLLLLLLLLLOOOOOOOOOOOOOOOBBBBBBBBAAAAAAAAALLLLLLLLLLL_______VARRRRRRRIIIIIIIABLEEEEEEEEE =
          99;

  final int
      abccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccc =
          99;

  void
      fooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooo() {

    boolean
        xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx =
            true;

    if (xxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxxx) {
      // some code
    } else {
      // some code
    }

    switch (GLLLLLLLLLLLLLLLLLLOOOOOOOOOOOOOOOBBBBBBBBAAAAAAAAALLLLLLLLLLL_______VARRRRRRRIIIIIIIABLEEEEEEEEE) {
      case abccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccc:
        fooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooooo();
        break;

      default:
        break;
    }

    System.out.println(
        "Helllllllllllllllllllllllllllllllllllllllllllllllllllllllllloooooooooooooooooooooooooooooooooo");
  }

  class Innnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnnneeeeeeeeeeeeeeeeeeeeeeeerrrrrrrrrrrrrrrrrrrr {}
}

Output:

$ java -jar .\checkstyle-10.17.0-all.jar -c .\google_checks.xml .\TestingColumnLimit.java
Starting audit...
[WARN] \TestingGoogleStyle\.\TestingColumnLimit.java:4: Line is longer than 100 characters (found 105). [LineLength]
[WARN] \TestingGoogleStyle\.\TestingColumnLimit.java:8: Line is longer than 100 characters (found 111). [LineLength]
[WARN] \TestingGoogleStyle\.\TestingColumnLimit.java:12: Line is longer than 100 characters (found 106). [LineLength]
[WARN] \TestingGoogleStyle\.\TestingColumnLimit.java:15: Line is longer than 100 characters (found 106). [LineLength]
[WARN] \TestingGoogleStyle\.\TestingColumnLimit.java:18: Line is longer than 100 characters (found 107). [LineLength]
[WARN] \TestingGoogleStyle\.\TestingColumnLimit.java:24: Line is longer than 100 characters (found 112). [LineLength]
[WARN] \TestingGoogleStyle\.\TestingColumnLimit.java:25: Line is longer than 100 characters (found 115). [LineLength]
[WARN] \TestingGoogleStyle\.\TestingColumnLimit.java:26: Line is longer than 100 characters (found 107). [LineLength]
[WARN] \TestingGoogleStyle\.\TestingColumnLimit.java:34: Line is longer than 100 characters (found 106). [LineLength]
[WARN] \TestingGoogleStyle\.\TestingColumnLimit.java:37: Line is longer than 100 characters (found 122). [LineLength]
Audit done.

Formatter tries to shrink down the line size but it still crosses the limit of 100.

@Zopsss
Copy link
Contributor Author

Zopsss commented Aug 2, 2024

we should revisit the working of LineLength module and add a new property to ignore long identifiers,

it is not that easy, LineLength become not Java Check, so it does not know anything about methods and variables.

points of attention ....aaaaaaaaaaa = and ...tttttttttt() { so tool does not break other rules for "=" and { to do line wrapping.

The only solution I see is to allow users to use suppressions to mark suppression in comment and explain reason https://checkstyle.org/google_style.html#Google_Suppressions, please play with this and lets choose what we would recomment for users to use if they run into this.

I think the SuppressWarningsFilter would be a good choice to recommend it to users, users' will just need to use SuppressWarnings annotation where they need suppression, something like:

  @SuppressWarnings({"LineLength"})
  final int abccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccccc = 99;

Edit: PR sent regarding this: #15426

@romani
Copy link
Member

romani commented Aug 2, 2024

@rdiachenko , do you agree with suppression?

@romani romani moved this from For Later to Todo in Refine Google Style Guide Aug 3, 2024
@Zopsss
Copy link
Contributor Author

Zopsss commented Aug 5, 2024

@rdiachenko

@romani
Copy link
Member

romani commented Aug 6, 2024

@Zopsss , while we are waiting for @rdiachenko , please send PR with update of Inputs annotations to show how it will looks like for user.
Please also update coverage table to mention that user should use extra code update to avoid violation on such edge cases.

Zopsss added a commit to Zopsss/checkstyle that referenced this issue Aug 6, 2024
…to show usage of suppression for long identifiers
@Zopsss
Copy link
Contributor Author

Zopsss commented Aug 6, 2024

done #15460 @romani

@rdiachenko
Copy link
Contributor

LineLength become not Java Check, so it does not know anything about methods and variables

I missed this point. Makes sense. I'm good with using suppression.

@romani romani moved this from Todo to In Progress in Refine Google Style Guide Aug 7, 2024
Zopsss added a commit to Zopsss/checkstyle that referenced this issue Aug 7, 2024
…to show usage of suppression for long identifiers
Zopsss added a commit to Zopsss/checkstyle that referenced this issue Aug 15, 2024
…to show usage of suppression for long identifiers
Zopsss added a commit to Zopsss/checkstyle that referenced this issue Aug 15, 2024
…to show usage of suppression for long identifiers
Zopsss added a commit to Zopsss/checkstyle that referenced this issue Sep 3, 2024
…to show usage of suppression for long identifiers
Zopsss added a commit to Zopsss/checkstyle that referenced this issue Sep 3, 2024
…to show usage of suppression for long identifiers
Zopsss added a commit to Zopsss/checkstyle that referenced this issue Sep 3, 2024
…to show usage of suppression for long identifiers
Zopsss added a commit to Zopsss/checkstyle that referenced this issue Sep 3, 2024
…to show usage of suppression for long identifiers
romani pushed a commit to Zopsss/checkstyle that referenced this issue Sep 7, 2024
…to show usage of suppression for long identifiers
romani pushed a commit that referenced this issue Sep 7, 2024
@romani romani added the bug label Sep 7, 2024
@romani romani moved this from In Progress to Done in Refine Google Style Guide Sep 7, 2024
@romani romani closed this as completed by moving to Done in Refine Google Style Guide Sep 7, 2024
@github-actions github-actions bot added this to the 10.18.2 milestone Sep 7, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
No open projects
Status: Done
Development

No branches or pull requests

3 participants