-
Notifications
You must be signed in to change notification settings - Fork 11.8k
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
Minor standards and cleanup additions. #581
Conversation
b59be03
to
99bd138
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
should we enforce spaces before function parens?
I'd go with always
. 👍
should we enforce semicolons or not? I'm partial to "no semicolons unless logically required".
Although I agree it looks prettier, I feel more comfortable if we consistently use semicolons. I'm open to discussion but I think it's definitely the most robust thing to do, and we should aim for robustness. Additionally, most of the repo is already using semicolons, and most of this PR consists of removing them, so it would help make it somewhat less "massive". 😃
Other than that, this is a welcome addition! Great work.
"mocha-lcov-reporter": "^1.3.0", | ||
"solidity-coverage": "^0.2.2", | ||
"truffle": "^4.0.0", | ||
"truffle-hdwallet-provider": "0.0.3" | ||
}, | ||
"dependencies": { | ||
"dotenv": "^4.0.0" |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Is this needed in dependencies
or could it go in devDependencies
? It only seems necessary if you want to run the tests.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It's required at runtime for truffle.js
, so theoretically it belongs in dependencies. But we really only use truffle for tests, since anyone using this project is including it as a dependency and using their own truffle.
It's up to you. We can also remove it, but being able to include a .env
file for things like MNEMONIC
and infura api keys is a lot easier than making sure to export them in your environment or include them in the command line when you run truffle
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Let's leave it this way. dotenv
has no production dependencies anyway.
@frangio added the rules and applied an autofix. We'll want to squash that commit correctly before merging, most likely. |
613d8df
to
ac38088
Compare
rebased and updated; sqashed the semicolon fix to 8662846 |
Thanks so much @shrugs! 🙌 |
Seeing this just now, @shrugs 👏 👏 |
Minor standards and cleanup additions.
--fix
applied).test.js
as a suffix to match traditional testing standardsmocks/
directory.Discussion
I'm very partial to the
space-before-function-paren
(always
) andsemi
rules (never
), but I understand if people think differently. Regardless of which direction we pick, we should enforce it, so the questions are:function () {}
vsfunction() {}
and (in classes)functionName () {}
vsfunctionName() {}
. I think the former looks prettier, but¯\_(ツ)_/¯
I'm aware that this is a massive PR and would mostly likely require rebasing any outstanding PRs to avoid merge conflicts. If anyone has strong opinions on that, please voice them.