Skip to content

Conversation

@Anmol202005
Copy link
Collaborator

Part of #41

Updated AbstractRecipeTest to ruCheckstyle and generate report directly rather than using an external report.

Also Changes made in upperEll recipe:

                if (((J.Literal) targetElement).getValueSource().matches("^[+-].*")) {
                    column++;
                }

when the literal has a sign prefix like -10000 or +10000, checkstyle violation point to the first digit (the 1) instead of the sign character. therefore added a check for it.

before:
result = out.length() - lineBreakIndex - 1;

after
result = out.length() - lineBreakIndex;

We were initially calculating column on zero-based index. fixed it now. (Checkstyle violations are (1-based))

@rdiachenko rdiachenko self-assigned this Jul 17, 2025
@rdiachenko
Copy link
Member

rdiachenko commented Jul 17, 2025

Also Changes made in upperEll recipe

Sounds like a bug and should be done in its own PR with added test case that catches it.

Please, resolve conflicts

@Anmol202005 Anmol202005 force-pushed the runCheckstyle branch 4 times, most recently from 6298672 to f8d84ed Compare July 18, 2025 16:37
@Anmol202005
Copy link
Collaborator Author

@rdiachenko Need help here !!
not sure why the test fails for window.

@Anmol202005 Anmol202005 force-pushed the runCheckstyle branch 3 times, most recently from 75e1a2a to dce5ab5 Compare July 18, 2025 17:29
@Anmol202005
Copy link
Collaborator Author

@rdiachenko Need help here !! not sure why the test fails for window.

fixed it!
The issue was that:

  • On Windows: System.lineSeparator() returns \r\n
  • Space.format() treats \r and \n as separate characters:
case '\n':
case '\r':
    if (inSingleLineComment) {
        // Processes single line comments
        comments.add(new TextComment(false, comment.toString(), prefix.toString(), Markers.EMPTY));
        prefix.setLength(0);
        comment.setLength(0);
        prefix.append(c);
    } else if (!inMultiLineComment) {
        prefix.append(c);
    } else {
        comment.append(c);
    }
    break;

Not sure how it passes initially!
will look raise a separate PR for this fix.

@rdiachenko
Copy link
Member

@Anmol202005 please do the following separation:

  1. PR that includes fix for UpperEll with corresponding test case. Create a separate issue and fix this under that issue
  2. PR that adds AbstractRecipeTestSupport, adds checkstyle dependency and implements verify method. Don't tauch AbstractRecipeTest and other tests
  3. PR that updates UpperEllTest to extend from AbstractRecipeTestSupport, remove custom reports and config files, and make it work end-to-end for UpperEllTest only. If you need anything from AbstractRecipeTest, copy it into AbstractRecipeTestSupport
  4. PR that updates HeaderTest similar to the previous point. At this point no tests use AbstractRecipeTest, so we can remove it

@Anmol202005 Anmol202005 force-pushed the runCheckstyle branch 2 times, most recently from edd0e6d to 115fa5b Compare July 19, 2025 18:26
@rdiachenko
Copy link
Member

@Anmol202005 please close this PR if we no longer need it

@rdiachenko rdiachenko removed their assignment Jul 20, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants