local_codechecker: Add sniffer to verify spaces around operators

[ankitagarwal]

Coding style doc says, we should always have spaces around operators. This sniff should verify that.
http://docs.moodle.org/dev/Coding_style#Control_statements

[danpoltawski]

Nice work Ankit! Please could you add some unit tests to verify the change?

[ankitagarwal]

It is a upstream sniffer, do we still need tests for it?

[danpoltawski]

Yeah to verify the rules match our rules & that future upstream updates don't break our rules (it's super easy to add the tests, look at the ones I just added :-))

[ankitagarwal]

Added tests now, hope that covers everything 👍

[danpoltawski]

I added a comment about the fixture file - I think its a bit misleading.

I ran the checker on moodlelib (recently cleaned up to not be throwing many errors) and it didn't find too many things. For loop assignments are an interesting one:

for·($i=0,·$j=count($lines);·$i·<·$j;·$i++)·{
$remainder·=·$totalsecs·-·($years*YEARSECS);

In any case, I would prefer to wait for Eloy for this (he has mostly been taking care of codechecker changes)

[ankitagarwal]

Thanks Dan,
I have update the comments on that page.
Also I have added two more sniffs,
-> Disallow consecutive multiple empty lines.
-> Make sure there is a new line at the end of the file.

[stronk7]

Hi, some comments:

• Does the sniff also check for multiple spaces in expressions/assignments being wrong? I think it's also something to verify and, the unit tests don't seem to cover them.
• About the multiple blank lines... I'm not sure if that's correct. It sounds to me that we allow 1-blank like everywhere (at developer decision to make code more readable), and 2-blank likes (no more!) only between classes. Perhaps I've dreamt that, but sounds to me a lot. So, under that idea, lines I think that comments like // 2 extra decimal places. should be allowed. #4 and Don't check XML files #5 in your tests should be throwing error (uhm... it sounds to me that we already were checking for that).
• About the EOL @ EOF, I'm the first thinking that ALL lines should have EOL. But this has been discussed in the past, and I think there was no consensus, so each developer is using its own way. We can reconsider it and make it a warning or so (and add it to the guide).

Edited: s/were/was

[ankitagarwal]

Hi Eloy,
Thanks for the comments.

• Yes it does. I will add some more asserts to cover those.
• I am generating a warning in the line before the blank lines, (Change boilerplate sniff, so that it does not insist on the word Moodle. #3) in this case. The main reason behind this was, editors cannot display an error on a blank line, while doing a realtime sniffing. If you prefer I can add a warning to all three lines (3,4,5) or change it to just (4,5). I was not aware we allow two blank lines in any situation. Am not sure if it is possible to sniff for the specific situation of separating two classes.
• I do remember reading it some where and was being pointed out by reviewers in my files a few times. However I cannot locate it in the code style doc. We can ignore this, if you wish.

Thanks

[stronk7]

Hi Ankit,

IMO, let's reduce this to the original "spaces around operators" goal (plz, add some test looking for multiple spaces as commented).

And let the blank lines and EOL @ EOF points not implemented for now. I don't think we'll raise any agreement about them and are really not important.

TIA!

[ankitagarwal]

Hi Eloy,
Asserts added. It works better than I expected. It ignores extra spaces before => and =, which we allow to align values, where as verifies it properly for other operators.
Thanks

[stronk7]

Merged, thanks Ankit!