Implemented Report and Config Parsner #16

Anmol202005 · 2025-06-14T10:20:31Z

Fixes: #15

Implemented Checkstyle Config Parser and violation report parser.

rdiachenko

@Anmol202005 please read this comment if you missed it #8 (comment)

We want to reuse parsing logic if it is already present

Anmol202005 · 2025-06-15T15:11:21Z

@Anmol202005 please read this comment if you missed it #8 (comment)

We want to reuse parsing logic if it is already present

@rdiachenko missed it, on it now.

romani · 2025-06-15T15:26:16Z

checksyle config parser is present in checkstyle jar, just reuse it.

Checkstyle report parser is missed in checkstyle jar, please look around in web, there should some parser already done.

romani · 2025-06-15T15:36:19Z

src/main/java/org/checkstyle/autofix/parser/CheckstyleConfigParser.java

+public class CheckstyleConfigParser {
+
+    /** Constant for the module XML tag name. */
+    private static final String MODULE_TAG = "module";


https://github.com/checkstyle/checkstyle/blob/effc726665baf6f2770ee072ca7d1d806704f25a/src/test/java/com/puppycrawl/tools/checkstyle/ConfigurationLoaderTest.java#L65

and it immeditate points to property handler to be provided.

Anmol202005 · 2025-06-15T15:43:00Z

@romani @rdiachenko

https://github.com/checkstyle/contribution/blob/master/patch-diff-report-tool/src/main/java/com/github/checkstyle/parser/CheckstyleReportsParser.java

this looks good !?

romani · 2025-06-15T15:49:10Z

yes

pom.xml

rdiachenko

@Anmol202005 I don't understand why we need all this complex logic? We should go with the smallest working solution and add functionality only when we need it. Please remove everything not relevant to parsing logic.

Additionally, where are the tests?

Anmol202005 · 2025-06-19T16:10:43Z

@rdiachenko done.

Anmol202005 · 2025-06-20T13:08:07Z

@rdiachenko I have refactored and made the implementation straight forward no more complex logic now.

with tests :)

pom.xml

src/main/java/org/checkstyle/autofix/data/CheckReport.java

src/main/java/org/checkstyle/autofix/data/CheckstyleRecord.java

src/main/java/org/checkstyle/autofix/parser/EmptyXmlEventReader.java

src/main/java/org/checkstyle/autofix/parser/StaxUtils.java

src/main/java/org/checkstyle/autofix/parser/CheckstyleReportsParser.java

src/main/java/org/checkstyle/autofix/data/CheckstyleRecord.java

src/main/java/org/checkstyle/autofix/parser/CheckstyleReportsParser.java

config/import-control.xml

src/main/java/org/checkstyle/autofix/parser/CheckstyleRecord.java

src/main/java/org/checkstyle/autofix/parser/CheckstyleReportsParser.java

src/main/java/org/checkstyle/autofix/parser/CheckstyleRecord.java

src/main/java/org/checkstyle/autofix/parser/CheckstyleReportsParser.java

timurt

Thanks for your work, added comments!

timurt · 2025-06-25T02:26:48Z

src/main/java/org/checkstyle/autofix/parser/CheckstyleRecord.java

+
+    private String fileName;
+
+    public CheckstyleRecord(int line, int column,


I see that the default value is -1 and also sometimes a violation contains only a line of information, missing column information

I would prefer to have an explicit nullable type, ex Integer instead of int

What do you think?

timurt · 2025-06-25T02:30:21Z

src/main/java/org/checkstyle/autofix/parser/CheckstyleRecord.java

+    public CheckstyleRecord(int line, int column,
+                            String severity, String source, String message, String fileName) {
+        this.line = line;
+        this.column = column;


Additionally, I might be wrong again

I recall that the AST line and column don't match one-to-one with the violation message, just because there are tabs

We have this conversion from the AST column to the violation column link

final int col = 1 + CommonUtil.lengthExpandedTabs( fileContext.fileContents.getLine(lineNo - 1), colNo, tabWidth);

Do you reckon we should store tabWidth information?

IMO we can consider it for another issue while implementing columns calculations in recipes.
additionally how are we going to input tabWidth information.

timurt · 2025-06-25T02:35:26Z

src/main/java/org/checkstyle/autofix/parser/CheckstyleRecord.java

+
+    private final String message;
+
+    private String fileName;


timurt · 2025-06-25T02:46:35Z

src/main/java/org/checkstyle/autofix/parser/CheckstyleReportsParser.java

+        while (attributes.hasNext()) {
+            final Attribute attribute = attributes.next();
+            if (FILENAME_ATTR.equals(attribute.getName().toString())) {
+                fileName = attribute.getValue();


It depends on preferences, but I would early-exit right away here without iterating till the end of the cycle

if (FILENAME_ATTR.equals(attribute.getName().getLocalPart())) { return attribute.getValue(); }

We do have a break;
am i missing something.

timurt · 2025-06-25T02:48:47Z

src/main/java/org/checkstyle/autofix/parser/CheckstyleRecord.java

+        this.severity = severity;
+        this.source = source;
+        this.message = message;
+        this.fileName = fileName;


By looking at the logic, it is quite possible that fileName is a nullable field

Is it okay? Or is it impossible?
Can we maybe explicitly add @Nullable annotation to nullable fields?

In a real report it can never be null.

@romani @timurt
We don't expect any field to be null unless there is a misconfiguration in the config.
Am i missing something??

rdiachenko · 2025-06-25T14:52:06Z

pom.xml

+        <!-- Checkstyle dependency for parsing Checkstyle configs -->
+<!--        <dependency>-->
+<!--            <groupId>com.puppycrawl.tools</groupId>-->
+<!--            <artifactId>checkstyle</artifactId>-->
+<!--            <version>${checkstyle.version}</version>-->
+<!--        </dependency>-->
+


Please remove it if not needed, we shouldn't have commented out code

Anmol202005 force-pushed the parser branch from a4fc553 to 855b590 Compare June 14, 2025 10:22

rdiachenko self-requested a review June 14, 2025 14:59

rdiachenko reviewed Jun 14, 2025

View reviewed changes

romani reviewed Jun 15, 2025

View reviewed changes

Anmol202005 force-pushed the parser branch 2 times, most recently from e9d5836 to 920b16e Compare June 18, 2025 11:24

romani reviewed Jun 18, 2025

View reviewed changes

pom.xml Outdated Show resolved Hide resolved

Anmol202005 force-pushed the parser branch from 920b16e to 888ef0d Compare June 18, 2025 16:25

rdiachenko requested changes Jun 18, 2025

View reviewed changes

Anmol202005 force-pushed the parser branch from 888ef0d to ac81201 Compare June 19, 2025 16:09

Anmol202005 force-pushed the parser branch from ac81201 to d64e7ea Compare June 20, 2025 13:06

rdiachenko self-assigned this Jun 20, 2025

rdiachenko reviewed Jun 20, 2025

View reviewed changes

Anmol202005 force-pushed the parser branch 2 times, most recently from 44410af to c770823 Compare June 22, 2025 13:43

rdiachenko reviewed Jun 22, 2025

View reviewed changes

Anmol202005 force-pushed the parser branch 2 times, most recently from 79c939d to ffa27fc Compare June 23, 2025 08:19

rdiachenko reviewed Jun 23, 2025

View reviewed changes

Anmol202005 force-pushed the parser branch from ffa27fc to 0a9a580 Compare June 23, 2025 21:54

rdiachenko reviewed Jun 24, 2025

View reviewed changes

rdiachenko requested a review from timurt June 24, 2025 18:48

timurt reviewed Jun 25, 2025

View reviewed changes

Implemented report and config parser

41ea853

Anmol202005 force-pushed the parser branch from 0a9a580 to 41ea853 Compare June 25, 2025 13:12

rdiachenko reviewed Jun 25, 2025

View reviewed changes


		private String fileName;

		public CheckstyleRecord(int line, int column,

Implemented Report and Config Parsner #16

Are you sure you want to change the base?

Implemented Report and Config Parsner #16

Conversation

Anmol202005 commented Jun 14, 2025

Uh oh!

rdiachenko left a comment

Choose a reason for hiding this comment

Uh oh!

Anmol202005 commented Jun 15, 2025

Uh oh!

romani commented Jun 15, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Anmol202005 commented Jun 15, 2025

Uh oh!

romani commented Jun 15, 2025

Uh oh!

Uh oh!

rdiachenko left a comment

Choose a reason for hiding this comment

Uh oh!

Anmol202005 commented Jun 19, 2025

Uh oh!

Anmol202005 commented Jun 20, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

timurt left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Anmol202005 commented Jun 20, 2025 •

edited

Loading