Skip to content

Issue #41: Updated AbstractRecipeTest to remove external report #44

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

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

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
Collaborator

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
Collaborator

@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
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