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.

Sign up for GitHub

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

Jump to bottom

Prefer const in test files #1117

Merged
merged 6 commits into from
Jul 26, 2018
Merged

Prefer const in test files #1117

merged 6 commits into from
Jul 26, 2018

Conversation

nventuro
Copy link
Contributor

  • var is removed and unallowed
  • let can only be used when the variable is re-assigned
  • const is used on all other cases

Covers part of #1091.

@nventuro nventuro requested a review from shrugs July 25, 2018 18:39
@nventuro nventuro added kind:improvement tests Test suite and helpers. labels Jul 25, 2018
@nventuro nventuro added this to the v1.12.0 milestone Jul 25, 2018
@nventuro
Copy link
Contributor Author

Many uses of let (e.g. when storing the created contract in a beforeEach can be removed by storing the contract in this, and some uses of const can be removed altogether (e.g. when reading from a getter), but that's coming in separate PRs to keep them easier to review.

shrugs
shrugs previously approved these changes Jul 26, 2018
View reviewed changes
Copy link
Contributor

@shrugs shrugs left a comment

Choose a reason for hiding this comment

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

🎉

come-maiz
come-maiz approved these changes Jul 26, 2018
View reviewed changes
Copy link
Contributor

@come-maiz come-maiz left a comment

Choose a reason for hiding this comment

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

@nventuro <3 awesome!
+1, with a PITA remark for following branches :)

.eslintrc
"promise/always-return": 0

"promise/always-return": "off",
"promise/avoid-new": "off",
Copy link
Contributor

Choose a reason for hiding this comment

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

I wouldn't like to delay this branch, and I love what you are doing with style.
But, this changes are not related to const. For the future branches, please split the things that are not related into small focused PRs.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Forgive me father for I have sinned :(

I know I shouldn't have, but I couldn't stand the ugly and unsorted eslint file. Didn't add any new rules other than the const related ones though.

test/LimitBalance.test.js

beforeEach(async function () {
lb = await LimitBalanceMock.new();
limitBalance = await LimitBalanceMock.new();
Copy link
Contributor

Choose a reason for hiding this comment

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

💯

@nventuro nventuro merged commit 567b773 into OpenZeppelin:master Jul 26, 2018
@nventuro nventuro deleted the prefer-const branch July 26, 2018 16:25
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@come-maiz come-maiz come-maiz approved these changes

@shrugs shrugs shrugs left review comments

Assignees
No one assigned
Labels
tests Test suite and helpers.
Projects
None yet
Milestone
v1.12.0
Development

Successfully merging this pull request may close these issues.
3 participants
@nventuro @come-maiz @shrugs