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

Checkstyle reporter #1129

Merged
merged 23 commits into from
Oct 10, 2024
Merged

Conversation

jonathan-dev
Copy link
Contributor

Added the possibility to export the linting results as checkstyle xml by using the --checkstyle flag on the lint subcommand.
The --teamcity flag takes precidence over --checkstyle.

The xml contents are written to stdout and thus can be forwarded to a file without containing the logs which are send to stderr (using 1>).

This feature was implemented in order to work with the jenkins warnings plugin

@jonathan-dev
Copy link
Contributor Author

#606

@codecov-commenter
Copy link

codecov-commenter commented Oct 4, 2024

Codecov Report

Attention: Patch coverage is 70.96774% with 27 lines in your changes missing coverage. Please review.

Project coverage is 95.01%. Comparing base (b4eac86) to head (70ff2c8).
Report is 31 commits behind head on main.

Files with missing lines Patch % Lines
internal/reporter/checkstyle.go 82.35% 6 Missing and 6 partials ⚠️
cmd/pint/ci.go 0.00% 8 Missing and 1 partial ⚠️
cmd/pint/lint.go 62.50% 3 Missing and 3 partials ⚠️
Additional details and impacted files
@@            Coverage Diff             @@
##             main    #1129      +/-   ##
==========================================
- Coverage   95.16%   95.01%   -0.15%     
==========================================
  Files         102      103       +1     
  Lines       10235    10355     +120     
==========================================
+ Hits         9740     9839      +99     
- Misses        346      360      +14     
- Partials      149      156       +7     

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

@prymitive
Copy link
Collaborator

Added the possibility to export the linting results as checkstyle xml by using the --checkstyle flag on the lint subcommand.

I, in general, don't want to add code that integrates with something that we don't run ourselves, as it just makes it impossible to refactor that code in the future.
I'm fine with adding a new reporter like this, as it's pretty much self contained, but only if it has good test coverage, which this one does.

The --teamcity flag takes precidence over --checkstyle.
The xml contents are written to stdout and thus can be forwarded to a file without containing the logs which are send to stderr (using 1>).

You need a file at the end, so why not make it --checkstyle=path.xml flag and leave --teamcity as is? In theory there could be any number of report files produced in a variety of formats but sending it all to stdout makes it impossible to enable multiple such reporters at once.

@prymitive
Copy link
Collaborator

@jonathan-dev
Copy link
Contributor Author

Added the possibility to export the linting results as checkstyle xml by using the --checkstyle flag on the lint subcommand.

I, in general, don't want to add code that integrates with something that we don't run ourselves, as it just makes it impossible to refactor that code in the future. I'm fine with adding a new reporter like this, as it's pretty much self contained, but only if it has good test coverage, which this one does.

The --teamcity flag takes precidence over --checkstyle.
The xml contents are written to stdout and thus can be forwarded to a file without containing the logs which are send to stderr (using 1>).

You need a file at the end, so why not make it --checkstyle=path.xml flag and leave --teamcity as is? In theory there could be any number of report files produced in a variety of formats but sending it all to stdout makes it impossible to enable multiple such reporters at once.

I change --checkstyle to be a string flag

@jonathan-dev
Copy link
Contributor Author

An e2e test like https://github.com/cloudflare/pint/blob/main/cmd/pint/tests/0158_lint_teamcity.txt would be great.

I created a test I am not comparing stderr because it was giving me trouble because of #20 and i don't think it is relevant.

@prymitive prymitive changed the title Checkstyle exporter Checkstyle reporter Oct 4, 2024
cmd/pint/ci.go Outdated
Name: checkStyleFlag,
Aliases: []string{"c"},
Value: "",
Usage: "Report problems using checkstyle xml.",
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Create a checkstyle xml formatted report of all problems to this path. ?

cmd/pint/ci.go Show resolved Hide resolved
if err != nil {
return err
if c.String(checkStyleFlag) != "" {
f, fileErr := os.Create(c.String(checkStyleFlag))
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Same here
Close() might error out so we should check for that

func (cs CheckStyleReporter) Submit(summary Summary) error {
dirs := sortByFile(summary)
var buf strings.Builder
buf.WriteString("<?xml version='1.0' encoding='UTF-8'?>\n")
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

encoding/xml can generate the entire XML file for you, that should be safer & more correct than manually crafting it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Sorry here I am not sure what you are referring to exactly. I see I could be using xml.Header.
Do you mean by having a custom struct that marshals to <checkstyle version='4.3'> making dirs marshal to <file name=.... and report to ?

output io.Writer
}

type Dirs map[string][]Report
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this doesn't have to be exported, does it?

@prymitive prymitive merged commit f01f208 into cloudflare:main Oct 10, 2024
18 checks passed
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.

3 participants