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

Closed

Conversation

K-Phoen
Copy link
Contributor

@K-Phoen 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 October 2, 2017 21:04
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).
Copy link
Member

Choose a reason for hiding this comment

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

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)
Copy link
Member

Choose a reason for hiding this comment

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

do not modify input parameter

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

Choose a reason for hiding this comment

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

question: why warning?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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);
Copy link
Member

Choose a reason for hiding this comment

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

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 ?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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
Copy link
Member

Choose a reason for hiding this comment

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

?

@SpacePossum
Copy link
Contributor

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
Copy link
Contributor

Choose a reason for hiding this comment

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

are we allowed to republish this under MIT here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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.

Copy link
Member

Choose a reason for hiding this comment

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

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'],
Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'll add a test today, then :)

@K-Phoen K-Phoen force-pushed the feature-checkstyle-reporter branch from 0c55140 to cf1d3f8 Compare October 5, 2017 11:26
@K-Phoen
Copy link
Contributor Author

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)
Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

don't we have all links with root / ?

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor

Choose a reason for hiding this comment

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

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

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Fixed :)

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

Copy link
Contributor

Choose a reason for hiding this comment

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

looking good now, thanks man 👍

@SpacePossum
Copy link
Contributor

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

@SpacePossum
Copy link
Contributor

LGTM 👍

@keradus
Copy link
Member

keradus commented Oct 6, 2017

Copy link
Member

@julienfalque julienfalque left a comment

Choose a reason for hiding this comment

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

LGTM 👍 except minor comments.

}

/**
* @covers \PhpCsFixer\Report\CheckstyleReporter::getFormat
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

ref #3133

)
);

$this->assertSame($actualReport, $expectedReport);
Copy link
Member

Choose a reason for hiding this comment

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

The expected value is supposed to be the first argument.

@keradus
Copy link
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
Copy link
Member

keradus commented Oct 9, 2017

ref checkstyle/checkstyle#5166

@K-Phoen
Copy link
Contributor Author

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
Copy link
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
Copy link
Contributor

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

@keradus keradus changed the title Feature checkstyle reporter Add checkstyle format Oct 10, 2017
@keradus
Copy link
Member

keradus commented Oct 10, 2017

Thank you @K-Phoen.

keradus added a commit that referenced this pull request 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 Oct 10, 2017
@K-Phoen K-Phoen deleted the feature-checkstyle-reporter branch October 10, 2017 21:29
@K-Phoen
Copy link
Contributor Author

K-Phoen commented Oct 10, 2017

Thanks for the review :)

@SpacePossum
Copy link
Contributor

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
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants