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 formatter #271

Merged
merged 6 commits into from Sep 20, 2019

Conversation

@Bu4ak
Copy link
Contributor

commented Sep 19, 2019

Q A
Bug fix? no
New feature? yes

Added checkstyle formatter. It needs for ci.
This reporting format also supported by phpstan (larastan).

@olivernybroe

This comment has been minimized.

Copy link
Collaborator

commented Sep 19, 2019

Ah this is pretty cool, never heard of checkstyle before.
Nice contribution! ❤️

composer.json Outdated Show resolved Hide resolved
* @param string $dir
* @param array<string> $metrics
*
* @throws Exception

This comment has been minimized.

Copy link
@nunomaduro

nunomaduro Sep 19, 2019

Owner

I would remove this.

Copy link
Owner

left a comment

@olivernybroe What do you think?

@nunomaduro

This comment has been minimized.

Copy link
Owner

commented Sep 19, 2019

@Bu4ak I don't know this format, can you add documentation about it on the website?

@Bu4ak Bu4ak marked this pull request as ready for review Sep 19, 2019
Copy link
Collaborator

left a comment

@olivernybroe What do you think?

I think it looks good. I cannot verify if he implemented it correctly, but it looks pretty solid and I'll just trust him that he knows the standard.

It looks like this is something often used with Jenkins and is the format is originally from Java. I think it's a good addition and something that we could keep having support for.
The format looks pretty basic, and as long as we don't change our API for the Details object, there shouldn't be that much maintaining.

@Bu4ak If you could fix the style tests that are failing and just add a few lines about it in the docs. Then I'll say it's ready to get merged. 😄

composer.json Outdated Show resolved Hide resolved
@dmitryuk

This comment has been minimized.

Copy link

commented Sep 19, 2019

If you could fix the style tests that are failing

@olivernybroe could you help to understand what is a problem?
I think the problem is that
‘24 Constructor of class
NunoMaduro\PhpInsights\Application\Console\Formatters\Checkstyle has an
unused parameter $input. ‘
But Json.php formatter doesn't use $input too and no any errors

public function __construct(InputInterface $input, OutputInterface $output)

@dmitryuk

This comment has been minimized.

Copy link

commented Sep 19, 2019

IMG_20190919_185950
Example checkstyle interface in teamcity (java code)

@olivernybroe

This comment has been minimized.

Copy link
Collaborator

commented Sep 19, 2019

@dmitryuk There are some errors.

One of them is you have an unused import of the Exception class.

To fix that one, do the same as we did for the json formatter, by adding a line in the phpstan file.

- '#NunoMaduro\\PhpInsights\\Application\\Console\\Formatters\\Json has an unused parameter \$input#'

You can test it locally by running composer test.

@dmitryuk

This comment has been minimized.

Copy link

commented Sep 20, 2019

@olivernybroe
@nunomaduro
All review issues are resolved.

Copy link
Collaborator

left a comment

Looks good.
Thanks for the contribution! 👍

@olivernybroe olivernybroe merged commit 0d21e1d into nunomaduro:master Sep 20, 2019
1 check passed
1 check passed
continuous-integration/travis-ci/pr The Travis CI build passed
Details
@dmitryuk

This comment has been minimized.

Copy link

commented Sep 20, 2019

Please release new version.

@dmitryuk dmitryuk deleted the RoboFinance:add-checkstyle-format branch Sep 20, 2019
@Jibbarth Jibbarth referenced this pull request Sep 23, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
4 participants
You can’t perform that action at this time.