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

[Feat] Add formatting support #201

Merged
merged 19 commits into from Aug 14, 2019

Conversation

@olivernybroe
Copy link
Collaborator

commented Jun 25, 2019

Q A
Bug fix? no
New feature? yes
Fixed tickets #61

This PR adds support for formatting the output is different formats.
You can now format to following outputs

  • JSON
  • CONSOLE

To use it simply add --format=json, or --format=console (console is default and fallback).

This PR also adds support for piping the result to a file (useful when formatting to json).
This means you can do phpinsights analyse--format=json > test.json and you will get the following result:
image
And the following data in the json file https://gist.github.com/olivernybroe/b68d0c66370a91486a138dcf8de102cd

One gotcha is when piping the result and using the console format without no interaction on, you have to press enter even though you cannot see the result as you are piping it to somewhere else. (don't know why you would pipe the result when using console format)

Another gotcha is that pipes does not support ANSI, so to get colored output while piping, you have to add the --ansi flag.

Adding a little todo with stuff I'll def forget if I dont add it here.

  • Add docs for formatting (remember gotchas in docs)
  • Add docs for contributing with a new formatter
  • Add json structure file for external tools to use as validation

@olivernybroe olivernybroe requested a review from Jibbarth Jun 25, 2019

@Jibbarth

This comment has been minimized.

Copy link
Collaborator

commented Jun 25, 2019

Impressive ! Great Job, I'm gonna review it.

By reading the gist, it seem there is a lack of information here and here

Is it possible to improve that ?

@Jibbarth
Copy link
Collaborator

left a comment

🥇

I left some comments, but it's really good.

Formatters are simple to understand and this allow us to possibly add more format in the future (xml, html... 😄)

About the json structure, there is some plugin/library able to parse it and generate some report ?

src/Application/Console/Analyser.php Show resolved Hide resolved
src/Application/Console/Formatters/Console.php Outdated Show resolved Hide resolved
src/Application/Console/Formatters/Console.php Outdated Show resolved Hide resolved
src/Application/Console/Formatters/Json.php Outdated Show resolved Hide resolved
src/Application/Console/Formatters/Json.php Outdated Show resolved Hide resolved
src/Domain/Insights/InsightFactory.php Show resolved Hide resolved
tests/TestCase.php Show resolved Hide resolved
@olivernybroe

This comment has been minimized.

Copy link
Collaborator Author

commented Jun 25, 2019

@Jibbarth Thanks for the review!
I'll go through the changes tomorrow 👌

So there is nothing that can parse this json, this is just a custom format that we specified ourself. However it opens up for the possiblity of tools doing that in the future. When we settle on a design, we should add a json structure file also.

Yeah, tried to make the formatter as simple as possible. We could actually add a xml formatter by wrapping the json formatter and converting the json to xml 👍

@nunomaduro nunomaduro self-requested a review Jun 26, 2019

.editorconfig Show resolved Hide resolved
@Jibbarth
Copy link
Collaborator

left a comment

For me it's ok.

For docs, do you want to continue on this PR or on another ?

@olivernybroe

This comment has been minimized.

Copy link
Collaborator Author

commented Jun 30, 2019

@Jibbarth let's do the docs in another PR. (I am totally fine with you doing it also 😏)

I won't be able to do any work before the end of the week as I am on vacation. ⛱️

@nunomaduro

This comment has been minimized.

Copy link
Owner

commented Jul 1, 2019

@olivernybroe Make sure you enjoy your vacations!

@Jibbarth

This comment has been minimized.

Copy link
Collaborator

commented Jul 2, 2019

@nunomaduro As you self-requested a review, I let you check this before going further, because it's a big change. Ok with that ?

olivernybroe added 3 commits Jul 4, 2019
@olivernybroe

This comment has been minimized.

Copy link
Collaborator Author

commented Jul 4, 2019

Just finished the docs and the schema file. This PR should be ready now :)

@nunomaduro

This comment has been minimized.

Copy link
Owner

commented Jul 4, 2019

Didn't had time to check this yet, do you mind of waiting? I am so sorry.

This was referenced Jul 5, 2019
@Jibbarth Jibbarth referenced this pull request Jul 5, 2019
@nunomaduro
Copy link
Owner

left a comment

Congrats @olivernybroe on this pull request, there is tons of improvement, you really nailed it.

Let's make sure we address my feedback before merging.

docs/contribute.md Show resolved Hide resolved
src/Application/Console/Analyser.php Outdated Show resolved Hide resolved
src/Application/Console/Commands/AnalyseCommand.php Outdated Show resolved Hide resolved
src/Domain/Details.php Outdated Show resolved Hide resolved
}
/**
* @param mixed $original

This comment has been minimized.

Copy link
@nunomaduro

nunomaduro Jul 7, 2019

Owner

You have no idea about what is coming for original?

This comment has been minimized.

Copy link
@olivernybroe

olivernybroe Jul 8, 2019

Author Collaborator

Well I can add the Sniff error class and keep mixed?

As right now the only data which is added in here is actually the sniff error, but for example with the phpstan error class a new type is added to this also.

We could also remove the original all together as we actually don't use it anywhere yet?

src/Domain/Insights/InsightFactory.php Show resolved Hide resolved
src/Domain/Insights/InsightFactory.php Show resolved Hide resolved
src/Domain/Reflection.php Show resolved Hide resolved
olivernybroe added 6 commits Jul 8, 2019
@Jibbarth Jibbarth referenced this pull request Jul 13, 2019
5 of 5 tasks complete
@olivernybroe

This comment has been minimized.

Copy link
Collaborator Author

commented Jul 20, 2019

@Jibbarth Let's wait with merging this until @nunomaduro approves it. 👍

@nunomaduro nunomaduro merged commit 3b174f1 into nunomaduro:master Aug 14, 2019

1 check failed

continuous-integration/travis-ci/pr The Travis CI build failed
Details
@nunomaduro

This comment has been minimized.

Copy link
Owner

commented Aug 14, 2019

Thanks buddy, impressive work on this. Great pull request and feature.

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.