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

Add checkstyle output format #2889

Closed
dmaciel opened this issue Jun 29, 2017 · 10 comments
Closed

Add checkstyle output format #2889

dmaciel opened this issue Jun 29, 2017 · 10 comments

Comments

@dmaciel
Copy link

dmaciel commented Jun 29, 2017

Hi

I know the primary goal of php-cs-fixer is to automatically fix code style and not to report the problems. Having said this, my company is starting using php-cs-fixer on the new code, and I believe will be interesting to track the advances through time with a Jenkins report

I already have a working version, if you are interested in this feature I can create a pull request and start a discussion from here

@keradus
Copy link
Member

keradus commented Jun 29, 2017

like php-cs-fixer fix --dry-run --report=junit ?

@dmaciel
Copy link
Author

dmaciel commented Jun 29, 2017

Yes and no, Junit can report the number of files with problems, while checkstyle reports show more detail about where are the errors located (file or folder), or you can group by type (The only thing that it's not possible is to get the line where the error is located)

The Junit report will serve to get the overall progress, while the checkstyle reports allow to have a breakdown off the same info

junit

checkstyle

@SpacePossum
Copy link
Contributor

SpacePossum commented Jun 29, 2017

Hi!

Your screenshots show a lot of details which seems of value to me :)
If making a POC in PR form is not a lot work I would be happy to look at it!

Based on some previous tickets I want to be upfront however that we always try to weight new (I/O related) features on the quality of the work, the amount of acceptance (# of users that would use it) and fit for the roadmap of this project, against the amount of work of reviewing and maintaining the feature (i.e. if the usage is high we expect more people to help out ;) ). This might not always be as transparent as one would like.

That said, having a PR would greatly help for us to see the concept in more detail, I'm especially interested in the output format itself and the tooling available to process such about (into, for example, the reports you provided screenshots of).

Thanks for coming here and talk to us and of course for using the fixer! :)

@keradus
Copy link
Member

keradus commented Jun 29, 2017

👍 singing under SpacePossum msg.

oooh, so checkstyle is actually a format, I thought t's just a generic name to call a report, cool.
Could you please say who can use it ? (like which CIs/Tools/plugins)

@K-Phoen
Copy link
Contributor

K-Phoen commented Sep 29, 2017

In the PHP ecosystem, this format is used by well-known tools like PHP_CodeSniffer or PHPStan.

Other languages also have tools emitting violations in this format (Java, Rust, JavaScript, …).

And as @dmaciel said, there is a widely used Jenkins CI plugin that know how to read these kind of files to generate super detailed reports.

Integrating PHP-CS-Fixer in Jenkins is already possible, thanks to the junit output format, but style violations are considered as test failures and it automatically fails the build. Being able to report these violations in a different format (the "checkstyle" one) would give us a better granularity when configuring the job failure conditions (in addition to being able to generate more helpful reports).

I guess that other CI servers would also be able to benefit from this output format, though I have to admit that I only use Jenkins.

If you do decide that this feature is needed, I can help with its implementation (and also maintenance, if needed).

@SpacePossum
Copy link
Contributor

I'm still carefully optimistic as I didn't see an example of the output format or some description of it.
The list of supported tooling makes me feel this would be great to have though :)

@keradus
Copy link
Member

keradus commented Sep 30, 2017

Yeah, we need format definition first, eg for JUnit we have https://github.com/FriendsOfPHP/PHP-CS-Fixer/blob/2.6/doc/junit-10.xsd .

Could you provide such definition (link to it) @K-Phoen ?

@K-Phoen
Copy link
Contributor

K-Phoen commented Sep 30, 2017

Unfortunately, I don't think that an "official" XSD is provided by the "original checkstyle".

I did found a few format definitions like this one from linkedin's repo, but none of them is "official".

@SpacePossum
Copy link
Contributor

LGTM 👍

@K-Phoen
Copy link
Contributor

K-Phoen commented Oct 1, 2017

I'll send a PR tomorrow then :)

keradus added a commit that referenced this issue Oct 10, 2017
This PR was squashed before being merged into the 2.8-dev branch (closes #3119).

Discussion
----------

Feature checkstyle reporter

As described in #2889, this PR introduces a *checkstyle* reporter.

I targeted the 2.7 branch, but this new reporter can easily be backported to the 2.2 branch (in fact, I already [cherry-picked the commits and backported it](https://github.com/K-Phoen/PHP-CS-Fixer/tree/backport-checkstyle-reporter))

Commits
-------

14a4ec6 Feature checkstyle reporter
@keradus keradus closed this as completed Oct 10, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

4 participants