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

Solium Integration, Linting Refactor #673

Merged
merged 9 commits into from
Jan 15, 2018
Merged

Conversation

shrugs
Copy link
Contributor

@shrugs shrugs commented Jan 15, 2018

  • πŸ“˜ I've reviewed the OpenZeppelin Contributor Guidelines
  • βœ… I've added tests where applicable to test my new functionality.
  • πŸ“– I've made sure that my contracts are well-documented.
  • 🎨 I've run the JavaScript linter (npm run lint:fix) and fixed all issues.

Fixes #656
Closes #556

πŸš€ Description

I cherry picked @azavalla's first commit to make sure that contribution is counted. Added npm script and travis integration as well as a secondary refactor commits.

I marked things like arg-overflow, security/no-inline-assembly, security/no-low-level-calls, and security/no-block-members as warnings. Note that warnings don't fail the travis script

For each rule still producing a warning:

  1. We can disable it altogether because OZ has reason to use this feature
  2. We can leave it as a warning to inform users (but some might be unnecessarily alarmed at the warnings generated)
  3. We can modify the code to fix it, which might change api surface / logic

Some issues we can't fix for backwards-compatibility reasons (namely, uppercase for public variables in ERC20), so I've added the ignore directives (but those are, ironically, being ignored, probably because it's a beta feature).

@shrugs shrugs added the review label Jan 15, 2018
@shrugs shrugs self-assigned this Jan 15, 2018
@shrugs shrugs force-pushed the feat/solium branch 2 times, most recently from 8495794 to 38d7bfc Compare January 15, 2018 20:28
@shrugs shrugs changed the title WIP: Solium Integration, Refactor Solium Integration, Refactor Jan 15, 2018
@shrugs
Copy link
Contributor Author

shrugs commented Jan 15, 2018

I fixed each set of lint errors iteratively in separate commits; to do a review, please check each commit separately (so you can make sure that that commit exclusively contains whitespace changes, for example).

@shrugs shrugs changed the title Solium Integration, Refactor Solium Integration, Linting Refactor Jan 15, 2018
@shrugs
Copy link
Contributor Author

shrugs commented Jan 15, 2018

A key note is that 0 logic has been changed (if it has, let me know!!) so this should have 0 external effect to things that depend on zeppelin-solidity.

@frangio
Copy link
Contributor

frangio commented Jan 15, 2018

Checked line by line, only aesthetic changes.

@frangio frangio merged commit ec2f7ba into OpenZeppelin:master Jan 15, 2018
@frangio frangio removed the review label Jan 15, 2018
@shrugs shrugs deleted the feat/solium branch January 15, 2018 21:45
ProphetDaniel pushed a commit to classicdelta/Smart-Contracts that referenced this pull request Mar 9, 2018
Solium Integration, Linting Refactor
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.

Merge solium PR and integrate into CI/testing
3 participants