Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add Linter (Solium) #59

Merged
merged 9 commits into from
Jul 24, 2017
Merged

Add Linter (Solium) #59

merged 9 commits into from
Jul 24, 2017

Conversation

Quazia
Copy link
Contributor

@Quazia Quazia commented Jul 17, 2017

WIP merge, this PR when finished will address issue #53. For more information on the progress see the issue. So far only non-breaking fixes have been applied but not all warnings/errors have been handled so far.

Currently WIP, all issues that don't create a breaking change will be handled
@izqui
Copy link
Contributor

izqui commented Jul 18, 2017

Thanks for taking the time, great work! :)

My brain hurts with the doubled indentation. How does everyone feel about it?

@readevalprint
Copy link
Contributor

@izqui I prefer the 4 spaces personally

@Quazia
Copy link
Contributor Author

Quazia commented Jul 18, 2017

I can switch it back to 2 spaces and change the Solium settings if that's the preference. I just went with defaults. Personally I think 4 spaces is easier to read but I'm an outsider.

@izqui
Copy link
Contributor

izqui commented Jul 18, 2017 via email

@readevalprint
Copy link
Contributor

@Quazia sorry, some major refactoring has been going on so there are a few conflicts

@Quazia
Copy link
Contributor Author

Quazia commented Jul 20, 2017

No problem. The non-trivial errors/warnings are still present as well as the parser error related to the solidity-parser repo. If you want to merge in in spite of this I can clean up the conflicts?

@readevalprint
Copy link
Contributor

Yeah we'd definitely appreciate it, we can try to fix the bigger issues in a different PR. I'd rather get this in before any other conflicts.

@Quazia
Copy link
Contributor Author

Quazia commented Jul 21, 2017

Alright, conflicts should be handled now. Should I add issues for the non-trivial errors and warnings?

@izqui
Copy link
Contributor

izqui commented Jul 21, 2017

I think last merge created some more conflicts

@Quazia
Copy link
Contributor Author

Quazia commented Jul 21, 2017

Alright I'll handle them first thing this morning, hopefully I'll be able to get ahead of it this time!

@Quazia
Copy link
Contributor Author

Quazia commented Jul 21, 2017

Alright, I believe everything should be sorted out now. The only rule I disabled was mixed_case as I assume since most instances are functions this would be a breaking error. The only other warning still present is:

In /home/quazia/Sites/quazia/aragon-core/contracts/kernel/organs/IOrgan.sol, line 6:
contract IOrgan is DAOStorage {
                              ^-- WARNING: Use of empty block statement {}

There's also still 2 instances of the parser error triggered by solidity-parser:

An error occurred while running the linter on /home/quazia/Sites/quazia/aragon-core/contracts/kernel/Kernel.sol:
Error: An error occured while parsing the source code:
SyntaxError: Expected "=:", "hex", "let", "{", "}", comment, end of line, number, string, or whitespace but "r" found. Line: 108, Column: 17

Hopefully this issue with the parser will be resolved.

In one place the statement id = id is used to circumvent an unused variable warning. This is sloppy but I didn't see any other way to avoid the warning and I believe leaving this warning on is useful.

@izqui izqui merged commit b1d67cd into aragon:master Jul 24, 2017
@Quazia Quazia deleted the soli branch July 24, 2017 10:35
@onbjerg onbjerg mentioned this pull request Sep 22, 2017
This pull request was closed.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants