Skip to content
This repository was archived by the owner on Jul 11, 2019. It is now read-only.

tests: add and follow most of the eslint rules #155

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

come-maiz
Copy link
Contributor

Requires #154

Brought to you with 🤓 by the #static task force.
Remember that if you have $opinions, you should report an issue on https://github.com/ZeppelinSolutions/code-style where we will discuss about it and then update the rules accordingly, if required.

Copy link
Contributor

@facuspagnuolo facuspagnuolo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks Leo! Left some comments, we can discuss this offline with the rest of the time if necessary

function getImplementation(string contractName) public view returns (address) {
function getImplementation(string contractName)
public view returns (address)
{
Copy link
Contributor

@facuspagnuolo facuspagnuolo Jun 1, 2018

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't understand why many lines are being fixed this way when we are working with a max-length of 120 Let's use 120

"quotes": ["error", "double"],
"no-empty-blocks": "off",
"indentation": ["error", 2],
"max-len": ["error", 79],
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

let's use 120

"no-debugger": 0,
"no-undef": 2,
"object-curly-spacing": [2, "always"],
"max-len": [2, 120, 2],
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

is this rule shadowing the one defined in .soliumrc.json?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

sorry, this is JS... I'd fix .soliumrc.json to 120

"no-constant": ["error"],
"security/enforce-explicit-visibility": ["error"],
"security/no-block-members": ["warning"],
"security/no-inline-assembly": ["warning"],
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

thinking about our Proxy contract, should we be warned since we are using inline-assembly to delegate calls?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, in this repo it's less useful. I like it because it forces us to have a good reason to use assembly, and we can use it as a reminder to put this good reason in a comment on the code.
I see this as a: WARNING: black magic ahead. Use with caution.
I would leave it because it's just three or four blocks of code, and hopefully it will not grow much, but no strong opinion here.

@come-maiz
Copy link
Contributor Author

@facuspagnuolo I have some reasons for 80 chars here:
OpenZeppelin/configs#3 (comment)
Feel free to reopen that issue so we can continue discussing there, or open a new one specific to the line length.
Also take into account that we are discussing if this should be an error or a warning: OpenZeppelin/configs#3 (comment)

Following the process we defied on #static, we should start using the current rules and in case of disagreement, discuss on the code-style repo. If we agree on 120, later I'll come back and adjust the code.

Thanks for reviewing!

@facuspagnuolo facuspagnuolo added the status:blocked Blocked issue label Jun 7, 2018
@facuspagnuolo
Copy link
Contributor

Waiting for the new approach we discussed offline to tackle linter rules

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
status:blocked Blocked issue
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants