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 format #3119

Closed
wants to merge 12 commits into
base: master
from

Conversation

Projects
None yet
4 participants
@K-Phoen
Contributor

K-Phoen commented Oct 2, 2017

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)

@keradus keradus changed the base branch from 2.7 to master Oct 2, 2017

README.rst Outdated
NOTE: When using ``junit`` format report generates in accordance with JUnit xml schema from Jenkins (see docs/junit-10.xsd).
NOTE: When using ``checkstyle`` format report generates in accordance with the "checkstyle" xml schema (see docs/checkstyle.xsd).

This comment has been minimized.

@keradus

keradus Oct 2, 2017

Member

please rephrase, maybe use bullets, no need to have 2 NOTE:.

maybe make it linkable, so Jenkins is link to junit-10.xsd and so ?

*
* @return \DOMElement
*/
private function addErrors(\DOMDocument $dom, \DOMElement $file, array $fixResult)

This comment has been minimized.

@keradus

keradus Oct 2, 2017

Member

do not modify input parameter

private function createError(\DOMDocument $dom, $appliedFixer)
{
$error = $dom->createElement('error');
$error->setAttribute('severity', 'warning');

This comment has been minimized.

@keradus

keradus Oct 2, 2017

Member

question: why warning?

This comment has been minimized.

@K-Phoen

K-Phoen Oct 5, 2017

Contributor

It's totally empirical: I had to choose a severity and I thought that style violations are not as critical as test failures, hence the severity defined as "warning".

In any case, whether the build should fail because of style violations can be configured directly in the CI, independently of their severity :)

{
$error = $dom->createElement('error');
$error->setAttribute('severity', 'warning');
$error->setAttribute('source', 'PHP-CS-Fixer.'.$appliedFixer);

This comment has been minimized.

@keradus

keradus Oct 2, 2017

Member

Wrong tool name, it's PHP CS Fixer, not PHP-CS-Fixer

Generated report contains only PHP CS Fixer related errors. Why we need prefix here ?

This comment has been minimized.

@K-Phoen

K-Phoen Oct 5, 2017

Contributor

For Jenkins to properly identify the source and type of violation, I had to specify these two information and replace the spaces in the name by -.

* @param \DOMDocument $dom
* @param \DOMElement $file
* @param array $fixResult
* @param bool $shouldAddAppliedFixers

This comment has been minimized.

@keradus
@SpacePossum

This comment has been minimized.

Member

SpacePossum commented Oct 2, 2017

Thanks @K-Phoen , looks promising! :)

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)

We accept new features on new releases only, so the current target master is spot on 👍 , still good to know it would be compat with older versions.

I hope to do good review tomorrow (if work allows it ;) ) !

you may not use this file except in compliance with the License.
You may obtain a copy of the License at
http://www.apache.org/licenses/LICENSE-2.0

This comment has been minimized.

@SpacePossum

SpacePossum Oct 3, 2017

Member

are we allowed to republish this under MIT here?

This comment has been minimized.

@K-Phoen

K-Phoen Oct 5, 2017

Contributor

I'm not sure.

We have to keep this file licensed under the Apache 2.0 license but we may have the right to dual license it.

This comment has been minimized.

@keradus

keradus Oct 6, 2017

Member

I would keep it as is, just add meta information about it with a link to origin

@@ -93,7 +93,7 @@ public function testAllFormatsCovered()
sort($formats);
$this->assertSame(
['json', 'junit', 'txt', 'xml'],
['checkstyle', 'json', 'junit', 'txt', 'xml'],

This comment has been minimized.

@SpacePossum

This comment has been minimized.

@K-Phoen

K-Phoen Oct 5, 2017

Contributor

As the checkstyle format does not include diffs, I don't think adding it to the test you mentioned is relevant :)

This comment has been minimized.

@SpacePossum

SpacePossum Oct 5, 2017

Member

ah yes, basically the test I think would be nice to have is to check that the OutputFormatter::escape($dom->saveXML()) is used (your code is correct, a test to prevent regression would be nice).

This comment has been minimized.

@K-Phoen

K-Phoen Oct 6, 2017

Contributor

I'll add a test today, then :)

@K-Phoen K-Phoen force-pushed the K-Phoen:feature-checkstyle-reporter branch from 0c55140 to cf1d3f8 Oct 5, 2017

@K-Phoen

This comment has been minimized.

Contributor

K-Phoen commented Oct 5, 2017

I updated the PR and rebased against master :)

README.rst Outdated
NOTE: the output for the following formats are generated in accordance with XML schemas
* ``junit`` follows the [JUnit xml schema from Jenkins](doc/junit-10.xsd)
* ``checkstyle`` follows the common ["checkstyle" xml schema](doc/checkstyle.xsd)

This comment has been minimized.

@SpacePossum

SpacePossum Oct 5, 2017

Member

these links don't seem to work, maybe the relative path should be done in another way?

This comment has been minimized.

@keradus

keradus Oct 5, 2017

Member

don't we have all links with root / ?

This comment has been minimized.

@SpacePossum

SpacePossum Oct 5, 2017

Member

as far as I can see in the README.rst proposed here we don't use it expect here

This comment has been minimized.

@SpacePossum

SpacePossum Oct 6, 2017

Member

@K-Phoen can you prefix the path with / as @keradus suggests to see if the links will work than?

This comment has been minimized.

@SpacePossum

SpacePossum Oct 6, 2017

Member

Thanks for updating, the links are still rendered as plain text. Maybe this is because the file is rst and not md?

This comment has been minimized.

@K-Phoen

K-Phoen Oct 6, 2017

Contributor

Fixed :)

You're right, I was writing markdown instead of reStructuredText.

This comment has been minimized.

@SpacePossum

SpacePossum Oct 6, 2017

Member

looking good now, thanks man 👍

@SpacePossum

This comment has been minimized.

Member

SpacePossum commented Oct 5, 2017

2 minors and this should be good to go IMHO :) 👍

@K-Phoen K-Phoen force-pushed the K-Phoen:feature-checkstyle-reporter branch from 7ed28b5 to c78b656 Oct 6, 2017

@SpacePossum SpacePossum added this to the 2.8.0 milestone Oct 6, 2017

@SpacePossum

This comment has been minimized.

Member

SpacePossum commented Oct 6, 2017

LGTM 👍

@keradus

This comment has been minimized.

Member

keradus commented Oct 6, 2017

@julienfalque

LGTM 👍 except minor comments.

}
/**
* @covers \PhpCsFixer\Report\CheckstyleReporter::getFormat

This comment has been minimized.

@julienfalque

julienfalque Oct 7, 2017

Member

The test class already has a @covers annotation, isn't this redundant?

This comment has been minimized.

@keradus
)
);
$this->assertSame($actualReport, $expectedReport);

This comment has been minimized.

@julienfalque

julienfalque Oct 7, 2017

Member

The expected value is supposed to be the first argument.

@keradus

This comment has been minimized.

Member

keradus commented Oct 8, 2017

to auto-fix few raised comments and allows me to not perform review manually, please rebase on current master to follow changes of #3133,
then 👍

@keradus

This comment has been minimized.

Member

keradus commented Oct 9, 2017

@K-Phoen K-Phoen force-pushed the K-Phoen:feature-checkstyle-reporter branch from 1b8823c to dd96fc4 Oct 10, 2017

@K-Phoen

This comment has been minimized.

Contributor

K-Phoen commented Oct 10, 2017

I rebased against master, is there anything left to do? (except wait for the "official" xsd to be written)

@keradus

This comment has been minimized.

Member

keradus commented Oct 10, 2017

IMO all good
no need to awaits for official xsd, we could switch when they will provide it

@SpacePossum

This comment has been minimized.

Member

SpacePossum commented Oct 10, 2017

@K-Phoen LGTM, thanks for all the work! 👍 :)

@keradus keradus changed the title from Feature checkstyle reporter to Add checkstyle format Oct 10, 2017

@keradus

This comment has been minimized.

Member

keradus commented Oct 10, 2017

Thank you @K-Phoen.

keradus added a commit that referenced this pull request Oct 10, 2017

feature #3119 Feature checkstyle reporter (K-Phoen)
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 Oct 10, 2017

@K-Phoen K-Phoen deleted the K-Phoen:feature-checkstyle-reporter branch Oct 10, 2017

@K-Phoen

This comment has been minimized.

Contributor

K-Phoen commented Oct 10, 2017

Thanks for the review :)

@SpacePossum

This comment has been minimized.

Member

SpacePossum commented Oct 10, 2017

More integration options like this help the current user-base but also open up the tool to a wider audience in roles like CI. As such, I feel this merged-PR is an addition of important to the codebase.
Thanks @K-Phoen much appreciated man!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment