Add linter

[izqui]

@onbjerg in #52: Solium could be used as the linter, it aims to comply with the official Solidity style guidelines.

[Quazia]

Are there any custom rules or are the standard rules okay for this? Also should a linter be added for non-solidity files and if so should the two linters be run through the same script (run lint) or separate scripts(run lint, run solium)?

Is changing the errors flagged through linting included in this issue?

[izqui]

I'd say to focus on Solidity linting first. The standard rules would be a good point to start.

Maybe we could add JS linting for the test files after

[Quazia]

Alright, I added the solidity linting and got about 90 errors and 500 warnings. I'm totally down to make the changes to pass the linter but don't want to go ahead on that if there are standards that you downright disagree with. I started fixing things that look like outright errors ( " 1 -1" vs " 1 - 1 ") but I'm leaving alone things that appear to be preferences ( opening brace on new line for functions with a returns statement in the function decleration for example ). Also it looks like Solium is throwing errors unrelated to the aragon-core code base. Once I get the warnings down I'll take a look into that and see if that's something that's easily resolved. I pushed just adding Solium which was very straight forward so let me know if you want me to put in a PR for just that, otherwise I'll wait until I clean some of these errors and put a PR in then.

[onbjerg]

@Quazia I think it would be neat if you opened to PR even if it's a work in progress. I'd like to know what sort of errors are thrown; could you perhaps include some examples?

[Quazia]

Yeah sure,

        if (!holder.send(returningMoney))  throw;
                                           ^-- ERROR: Consequent should exist exactly on the line after condition.

These errors are pretty common, this and the 4 space tabs over 2 space tabs are probably two of the most common

    function BoundedStandardSale(address _daoAddress, uint8 _tokenId, uint256 _min, uint256 _max, uint256 _price, uint64 _closeDate, string _title) {
    ^-- WARNING: Function 'BoundedStandardSale': in case of more than 3 parameters, drop each into its own line.

This is one that I think makes sense. I think it's a lot easier to read when they're dropped down.

In /home/quazia/Sites/quazia/aragon-core/contracts/kernel/organs/VaultOrgan.sol, line 321:
    modifier check_blacklist(address _token) {
    ^-- WARNING: 'check_blacklist' doesn't follow the mixedCase notation

I haven't been resolving the mixedCase notation where it's a function name as I don't want to create a breaking change elsewhere. I don't know if the smart solution to this would be to allow snake_case in the linter or to just standardize to mixedCase.

            (sig == GET_TOKEN_BALANCE_SIG)||
             ^-- ERROR: There should be no comments between right side and '||'.
             ^-- ERROR: There should be no comments between right side and '||'.
             ^-- ERROR: There should be no comments between right side and '||'.
             ^-- ERROR: There should be no comments between right side and '||'.
             ^-- ERROR: There should be no comments between right side and '||'.
             ^-- ERROR: There should be no comments between right side and '||'.
             ^-- ERROR: There should be no comments between right side and '||'.
             ^-- ERROR: There should be no comments between right side and '||'.
             ^-- ERROR: There should be no comments between right side and '||'.

I'm having difficulty resolving this one in a way that only changes syntax so I moved past it for the time being, the same error is thrown in all of the canHandlePayload functions across the Organ contracts. In addition, some of the canHandlePayload functions are causing the parser to crash but I've found just adding parentheses seems to resolve the parser errors.

48 errors, 211 warnings found.

I've gotten about half of the errors/warnings handled but I'm sure that as I fix the parser errors more will pop up.

I have a txt file of the lint output if you'd like to take a look and I'm going to put a PR in as a WIP as requested. I'd say the vast majority of warnings and errors fit into one of the categories above.

[Quazia]

Currently there is a parser error for some of the assembly blocks for example:

solidity-parser ./contracts/kernel/organs/ApplicationOrgan.sol 
Expected "=:", "hex", "let", "{", "}", comment, end of line, number, string, or whitespace but "r" found. Line: 33, Column: 13

This is due to the following issue in ConsenSys/solidity-parser "Missing assembly rules: for, switch, function #83"

The parsing issue presents in multiple contracts when a return statement is used in an assembly block.

Going to proceed for the time being and if needed maybe try and resolve this issue in the solidity-parser

[izqui]

Awesome! Btw, just a heads up, old-contracts is going to be deleted from the repo, it is just there as a quickly accessible reference

[onbjerg]

Linter added in #59.