Support for subtypes

[technosophos]

As you know, I've been working to optimize the PHP syntax checking and code style validation with php -l and phpcs. I'm trying to figure out a way to visually distinguish syntax errors from coding standards violations.

This pull request is an effort to add "subtype" support that allows secondary checkers (like coding style validators) to use a different Sign than the standard checker. Only signs are impacted by this patch -- all other displays are left as they were (I think. I intended to leave them as they were). The code should be such that it does not require existing checkers to change.

Screenshot:
https://img.skitch.com/20120127-f9yyympua9e16d6tssdqaynp52.png

In the attached patch, I added support for a subtype called Style, which uses the SyntasticStyleError and SyntasticStyleWarning signs. I implemented PHP support and tested backward compatability with Ruby.

I think this method should make it relatively easy (though perhaps not trivially so) to add more subtypes.

I know this is sort of a change in the direction you've taken, and if you don't like this, I will not feel bad about having the pull rejected. I know that what's useful for me may not be generally desirable for everyone else.

[scrooloose]

Hey, ive been thinking about this for the last few days and I think it is a good idea in principle. The main objection that I would have to it is that it could end up never being used for anything other than phpcs.

However, since the code is very simple I will pull it if the following mods are made:

1. In the php checker, you have removed the check for the empty error list before invoking phpcs. There was a good argument for adding this in only run phpcs if the file has no syntax errors. #155. Is there a reason to revert this?
2. remove this function s:SubtypeMasksType(error, llist). It is not needed since we have no way of knowing how future syntax checkers will prioritize subtyped errors. Also, in the current php checker, there will never be phpcs and php -l errors occurring at the same time (assuming the check in point 1 is re-added).

[technosophos]

First, I think this is a way of incorporating tools for other languages like PMD (http://pmd.sourceforge.net/) and Checkstyle (http://checkstyle.sourceforge.net/). It's not a PHP-only problem. We just have the dubious distinction of being the first to whine about it to you. ;-)

Regarding your two comments:

#1: the example for 155 has been removed, so I don't know what horrible result initially triggered this bug, but my own usage has indicated that the vast majority of time it makes sense to run both, even if one fails. I'd offer two reasons for this:

• Syntax errors are often known at the time when one saves a document. e.g., I might start writing if ( and then pause, :w, and then go back to writing. I don't want this to toggle me from one "mode" to another.
• Changing markers frequently is confusing. At moment t, my file looks like it has one error. Then I fix that error, and at t+1 I have a whole bunch of new errors.

Clearly, horrendous and unusable output from phpcs would override both of my points above. But if we're talking about a few spurious formatter errors based on a single syntax error, I think I'd be inclined to select continuity.

Again, I'm very flexible about this. I can only offer my own opinion as evidence and state that I commented out the empty check soon after it. I thought it was just a stop-gap until something like subtype checking was added.

#2: If we want to leave the empty() check in, I will revert this out. As it is, I think the design was built on the assumption that the first check is a true syntax check, and all subsequent ones are for non-critical aspects of checking.

[AD7six]

the phpcs library can throw hundreds of undefined index errors depending on the combination of what standard you are using, the exact parse error and where it occurs in the token stack.

The gist that has since been deleted (it infact contained some ~sensitive info) was 3 thousand lines of php errors and a report at the end of it with about 5 phpcs errors. It would take literally minutes for syntasticvim to parse each line of that output, before finally getting to the report and then painting the few syntax errors found.

Maybe there's a different way of looking at it - code errors coming out of a linting/standard tool are written to stderr - if stdout is the only thing processed by syntastic, the original reason not to append phpcs output to what php -l shows goes away (though personally, if a file has a parse error all bets are off and running a code formatting tool is a bit pointless)

Edit: well that doesn't work - php errors coming out of phpcs are sent to both stdout and stderr - you can't simply 2> /dev/null to get only the report it would seem.

[technosophos]

Yikes! Yeah, that would be REALLY annoying to have to wait a few minutes.

I'll add that empty check back in and re-commit. (And I will remove the s:SubtypeMask function, too).

[AD7six]

in stereo baby! (view ticket via the web)

[technosophos]

By removing SubtypeMasksType() this means that any other checker that uses multiple subtypes will have to do the same sort of empty() check that PHP currently does.

I've made both requested changes now.

[technosophos]

Ha! Sorry... my habit of referencing ticket numbers in git commits has come to haunt me.

Or maybe I just repeat myself a lot.
Or maybe I just repeat myself a lot.