-
Notifications
You must be signed in to change notification settings - Fork 53
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
Checkstyle reporter #1129
Conversation
Codecov ReportAttention: Patch coverage is
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. |
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.
You need a file at the end, so why not make it |
An e2e test like https://github.com/cloudflare/pint/blob/main/cmd/pint/tests/0158_lint_teamcity.txt would be great. |
I change |
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. |
cmd/pint/ci.go
Outdated
Name: checkStyleFlag, | ||
Aliases: []string{"c"}, | ||
Value: "", | ||
Usage: "Report problems using checkstyle xml.", |
There was a problem hiding this comment.
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.
?
if err != nil { | ||
return err | ||
if c.String(checkStyleFlag) != "" { | ||
f, fileErr := os.Create(c.String(checkStyleFlag)) |
There was a problem hiding this comment.
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
internal/reporter/checkstyle.go
Outdated
func (cs CheckStyleReporter) Submit(summary Summary) error { | ||
dirs := sortByFile(summary) | ||
var buf strings.Builder | ||
buf.WriteString("<?xml version='1.0' encoding='UTF-8'?>\n") |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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 ?
internal/reporter/checkstyle.go
Outdated
output io.Writer | ||
} | ||
|
||
type Dirs map[string][]Report |
There was a problem hiding this comment.
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?
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