Create report that will show code with error #419
Comments
|
This would certainly be an interesting report type, but would never be the default. Even for a moderately complex file, the number of errors, and thus number of output lines, would be far too great. The report would border on unreadable unless the number of errors is quite small (probably less than 10). But it could certainly be an optional report, and you can set your default report format to be whatever you want. In my opinion, this sort of report is better suited to being integrated into an IDE, where action can be taken to fix the issues themselves. There are plugins available for most IDEs to do this, and they typically use the existing developer-friendly reports such as JSON, XML or CSV. |
gsherwood
added
Status: awaiting feedback
Type: enhancement
and removed
Status: awaiting feedback
labels
|
I've added a new report type in the 3.0 version for this. Here is an example: For this code: <?php
$var = array(
'one' => 1,
'one1' => 1,
'one11' => 1,
'\000' => 1,
);Running it over the Squiz standard with the full report ( Using the new code report ( Note that I have also set the tab width to 4 ( If you have any suggestions for the report, please let me know. |
You can use single separator to get same effect. or maybe separator that looks differently I presume in case of |
|
The reason I used the double separator was so that is was clear which error list was for each line. Once you get a long list of errors, the report becomes much harder to read. Compare this: To this: If you were just looking at that report, you wouldn't know if the errors were for the code snippet below or above (assume there is more content above and below this snippet). Of course we know the error are for the snippet below, and you can tell if you look at the top of the report, but I wanted to make sure it wasn't confusing for other people. An alternative approach would be to use a single separator, but include the line numbers in the error messages: What do you think about that? |
Yes. It looks exactly the same as it does in the standard report. Like that report, it also removes the |
|
Without line numbers shown it was hard to guess where relevant code snippet is located (above errors or below them). What about |
|
Is this more like what you are thinking? |
|
Now when you put |
|
I might just make it this then, because there is no need for double lines if I put the line numbers back into the error messages: |
|
One more option - leave out the separators all together: |
|
I like it. It does differ greatly from initial version, but different doesn't mean it's bad. |
Which option are you talking about? Line numbers with separators or no separators? I think I like the one in this comment best because I think it fits in with the style of the other reports as well: #419 (comment) |
I agree with you. User can clearly see line number in error message and in the code and this way can detect which code related to what error messages without knowing if error messages come after or before code. |
|
I've committed a change to the report format. The original code now outputs a report looking like this: Looks good? |
|
Yes. I suppose this was report after auto-fixing, because I no longer see TAB sings in there used for indentation along with SPACEs as in previous examples. |
|
I am probably late to the party, but have you perhaps considered reversing the code-erorrs order? I mean now the errors are printed out before the code - before you know whats wrong, so in my case I would usually have to glance at them, then at the code, but go for specifics back to the code. If the order would be reversed in plenty (in my case most of them) I would know what the error is just by looking at the line of code - because the purpose of CS for me is "reminding" me that I do everything right (or that I made a typo), not that I don't know, how it should look like when I think about it. |
That depends. If that is an ERROR, then you must fix it. If it's a WARNING, then you're recommended to fix it, but it's not necessary. |
In ideal world yes. But at my company we're created coding standard because people have no idea what is wrong with their code until I said them what's wrong. |
|
I am not speaking about error vs warning, I am talking about workflow ;)
Even if you don't know what exactly is wrong with the code, you get references after you have seen it - with reference to the specific line. In the current shape you give the references "in advance" - before the user has seen it. |
|
IMO coding standard enforcing tool, like PHP_CodeSniffer is for cases, when developers don't know how things needs to be done and make mistakes and then PHP_CodeSniffer finds these mistakes and reports them. It's important for developer to know what mistake is rather then seeing broken code first and then realizing what mistake was made. |
|
I don't think having the errors on the top or bottom is going to make much difference to glancing. Plus, the errors are the main content and the code snippet is in support of them. So I believe the errors should come first, followed by the supporting material. I'm going to close this as I'm now happy with the report format. Thanks for the feedback. |
|
Ok, it's a great improvement anyway, thanks! :) |
In Phabricator linting tool the default reporting way is to also show relevant code piece where error is happend with usual 3 lines of context. This drastically reduces switching between Terminal (Mac) and IDE when fixing errors.
Example:
This is file, that does this: https://github.com/phacility/arcanist/blob/master/src/lint/renderer/ArcanistConsoleLintRenderer.php
IMO such report might become new default report type.
The text was updated successfully, but these errors were encountered: