Skip to content
This repository has been archived by the owner on Jun 11, 2024. It is now read-only.

Apply eslint-rule no-shadow - Related to #1597 #2583

Merged
merged 16 commits into from
Dec 4, 2018

Conversation

diego-G
Copy link

@diego-G diego-G commented Nov 27, 2018

Review checklist

Copy link
Contributor

@ManuGowda ManuGowda left a comment

Choose a reason for hiding this comment

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

@diego-G overall the purpose of the PR is great, but in my opinion at some places the naming are not the convincing, I have left few comments and feedback, please feel free to go over it and if there is room for improvement please do them.

app.js Outdated Show resolved Hide resolved
app.js Outdated Show resolved Hide resolved
helpers/sort_by.js Outdated Show resolved Hide resolved
logger.js Outdated Show resolved Hide resolved
logic/account.js Outdated Show resolved Hide resolved
modules/peers.js Show resolved Hide resolved
modules/rounds.js Outdated Show resolved Hide resolved
test/common/db_sandbox.js Outdated Show resolved Hide resolved
test/common/db_seed.js Outdated Show resolved Hide resolved
test/common/modules_loader.js Outdated Show resolved Hide resolved
Copy link
Contributor

@MaciejBaj MaciejBaj left a comment

Choose a reason for hiding this comment

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

Using underscores in names is also not recommended and conflicting with the other ESLint rule - https://eslint.org/docs/rules/no-underscore-dangle.

@diego-G
Copy link
Author

diego-G commented Nov 28, 2018

@maciej the rule we apply comes straight from AirBnb repo. My Lint doesn't show any conflicts. Could you point me to the place you refer?
For reference: https://github.com/airbnb/javascript/blob/17e0454672f784772c5f5ab052523a93229d9adf/packages/eslint-config-airbnb-base/rules/style.js#L371

api/controllers/node.js Outdated Show resolved Hide resolved
api/fittings/lisk_params_validator.js Outdated Show resolved Hide resolved
logic/account.js Outdated Show resolved Hide resolved
logic/account.js Outdated Show resolved Hide resolved
logic/broadcaster.js Show resolved Hide resolved
logic/multisignature.js Outdated Show resolved Hide resolved
logic/transaction.js Show resolved Hide resolved
ManuGowda
ManuGowda previously approved these changes Nov 29, 2018
Copy link
Contributor

@MaciejBaj MaciejBaj left a comment

Choose a reason for hiding this comment

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

Please use the meaningful variable names.

@diego-G diego-G requested review from MaciejBaj and removed request for 4miners November 29, 2018 13:05
Copy link
Contributor

@nazarhussain nazarhussain left a comment

Choose a reason for hiding this comment

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

If you exclude that controllers/node.js everything else seems pretty good to me.

api/controllers/node.js Outdated Show resolved Hide resolved
nazarhussain
nazarhussain previously approved these changes Nov 29, 2018
ManuGowda
ManuGowda previously approved these changes Nov 30, 2018
@diego-G diego-G dismissed stale reviews from ManuGowda and nazarhussain via 794e28e November 30, 2018 15:06
@MaciejBaj MaciejBaj removed their assignment Nov 30, 2018
nazarhussain
nazarhussain previously approved these changes Dec 3, 2018
ManuGowda
ManuGowda previously approved these changes Dec 3, 2018
@MaciejBaj MaciejBaj merged commit ad7d5d4 into development Dec 4, 2018
@MaciejBaj MaciejBaj deleted the 1597-apply_no-shadow branch December 4, 2018 16:39
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants