Start to work on generic issue reporters by K-Phoen · Pull Request #235 · ovr/phpsa

K-Phoen · 2016-10-11T10:37:34Z

Hey!

Type: new feature

Link to issue:

This pull request affects the following components (please check boxes):

In raising this pull request, I confirm the following (please check boxes):

I have read and understood the Contributing Guidelines.
I have checked that another pull request for this purpose does not exist.
I wrote some tests for this PR.

Small description of change:

This PR starts to make the output generation generic. We should be able to generate several output formats depending on what the user wants (xml, json, text, …).
For now, I only introduced a generic way to transform issues to JSON/XML, other. In future commits, I'll work on a solution to combine several outputs, manage IO, etc.

Thanks

ddmler · 2016-10-11T10:57:00Z

I assigned you to issue #18 then. It's HTML output. For start it would be enough to just wrap it in some html tags, the styling can be done later

ovr · 2016-10-11T10:58:40Z

@@ -0,0 +1,38 @@
+<?php
+
+namespace PHPSA\Report;


We should put Report inside Analyzer directory
And seems Issue's classes to this dir

but syntax error issues belong to compiler not analyzer

K-Phoen · 2016-10-11T21:37:03Z

@ovr, @ddmler I reworked the initial commit a bit.

This PR now introduces the notion of reporter (a class in charge of reporting issues to the user), that can use different formatters and outputs.

In order to make it work, I also had to clean a few things, hence the number of modified lines.

ddmler · 2016-10-11T21:52:45Z

                $symbol = $context->getSymbol($var->name);
                if (!$symbol) {
-                    $symbol = new \PHPSA\Variable(
+                    $symbol = new \PhpSA\Variable(


even though it works I think we should stick to PHPSA all uppercase?

PHPSA should be in uppercase
Because it's abbreviation

Yep 👍
This modification wasn't intended.

ddmler · 2016-10-11T22:04:24Z

+        return $output . "\n";
+    }
+
+    private function formatNotice(Issue $issue)


Can't we change that to:
Filename
line: issue 1 [checkname]
code of issue 1
line: issue 2 [checkname]
code of issue 2
next Filename
...
Also we want to add language level errors 👍

In this PR, I'll only provide the architecture to write different outputs. We'll be able to add new or update existing ones later :)

ovr · 2016-10-12T00:59:54Z

            'description' => $this->description,
            'location' => $this->location->toArray(),
            'categories' => $this->categories,
+            'blame' => $this->blame,


It's not needed there, toArray is a special method for CodeClimate

Having a method that is specific for CodeClimate in a class that's not specific is a bit weird I think.

Moreover, shouldn't we implement CodeClimate as another output format?

ovr · 2016-10-12T01:01:57Z

    protected $type;

+    /** @var NodeAbstract The AST node where the variable is declared */
+    protected $declarationStmt;


ovr · 2016-10-12T01:03:40Z

+        $json = json_encode($issue->toArray());
+
+        if (!$this->isFirstIssue) {
+            $json = ', '.$json;


I suggest to use format as CodeClimate
echo $json . chr(0);

ovr · 2016-10-12T06:26:50Z

+     * @param string|null $filepath
+     * @return bool
+     */
+    protected function addIssue($type, $message, $line, $filepath = null)


IssueLocation in near feature will be different and will support blocks
and half of line

Like https://github.com/codeclimate/spec/blob/master/SPEC.md#locations

Btw, I suggest to pass Issue, because this will be rly hard to pass 100500 parameters to customize Issue
For example cutegories, IssueLocation

I updated the method definition to use an IssueLocation object.

@K-Phoen But how to pass category? Myabe lets use Issue?

Or another way to split methods like

->styleNotice ->securityNotice ->performanceNotice

m?

Yep yep, done (I didn't see your previous comment before updating the code).

The Context class already has too many methods/responsibilities. I just wanted to initiate a bit of cleaning :)

but anyway main question how pass categories? Lets add a parameter with default value to notice method, will be helpfull to pass normal category or categories of the Issue

And for errorLanguage lets pass Issue::CATEGORY_BUG_RISK to Issue

K-Phoen · 2016-10-13T07:50:27Z

@ovr If that sounds good to you, I think that I will modify the codeclimate-related code to be just another output format. I'll create an issue for that and do it in a separate PR.

Also, I think that the Context class looks to much like a god object (it holds too much responsibilities) and that we should think about what we want to do about it. If we actually want to do something about it.

WDYT?

Start to work on generic issue reporters

c53cce9

K-Phoen force-pushed the reporters branch from 5786d86 to c53cce9 Compare October 11, 2016 10:40

ovr reviewed Oct 11, 2016

View reviewed changes

Kévin Gomez added 2 commits October 11, 2016 23:32

Introduce a reporter class and report formatters

ac5d827

Update tests fixtures

6f372a5

ddmler reviewed Oct 11, 2016

View reviewed changes

ovr reviewed Oct 12, 2016

View reviewed changes

K-Phoen force-pushed the reporters branch 3 times, most recently from b9d125e to de8145a Compare October 12, 2016 06:39

Fix tests

d09e520

K-Phoen force-pushed the reporters branch from de8145a to d09e520 Compare October 12, 2016 06:49

ovr added this to the Release 0.6.1 milestone Nov 12, 2016

Conversation

K-Phoen commented Oct 11, 2016 • edited by ddmler Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

ddmler commented Oct 11, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

K-Phoen commented Oct 11, 2016

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ddmler Oct 11, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

ovr Oct 12, 2016 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

K-Phoen commented Oct 13, 2016

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

K-Phoen commented Oct 11, 2016 •

edited by ddmler

Loading

ddmler Oct 11, 2016 •

edited

Loading

ovr Oct 12, 2016 •

edited

Loading