Start to work on generic issue reporters #235

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

src/Report/JsonReporter.php

@@ -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

src/Compiler/Expression/Assign.php

                    continue;
                }

                $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

src/Report/Formatter/TextFormatter.php

+        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

src/Issue.php

@@ -130,6 +132,7 @@ public function toArray()
            '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

src/Variable.php

@@ -59,17 +60,22 @@ class Variable
     */
    protected $type;

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


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

src/Report/Formatter/JsonFormatter.php

+        $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

src/Context.php

+     * @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

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Start to work on generic issue reporters #235

Start to work on generic issue reporters #235

Start to work on generic issue reporters #235

Are you sure you want to change the base?

Start to work on generic issue reporters #235

Conversation

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment