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

Add no-shadow #370

Open
fregante opened this issue Mar 15, 2018 · 4 comments
Open

Add no-shadow #370

fregante opened this issue Mar 15, 2018 · 4 comments

Comments

@fregante
Copy link
Member

I thought xo had this one at some point: https://eslint.org/docs/rules/no-shadow

const love = get();
for (const love of feelings) {
	// what is love?
}
@sindresorhus
Copy link
Member

Same as #280.

I think I removed it at some point because it was buggy and annoying, but I'm willing to add it back and try again.

@pvdlg Thoughts?

@sindresorhus
Copy link
Member

// @zanona

@pvdlg
Copy link
Contributor

pvdlg commented Mar 15, 2018

I find the ability to shadow variables quite convenient in some cases. For example with reduce I usually name the accumulator and the resulting object with the same name:

const array = [];
const someResult = array.reduce((someResult, value) => {
  // ...
}, {});

It's convenient because I never know how to name the accumalator^^. And it makes sense (at least to me) to name the variables that way as someResult is the final result and someResult in the reducer is the final result being constructed. Both someResult variables represent the same "thing", but at a different moment in time.

It's also convenient when using find:

const params = [];
const param = params.find(param => param === 'something');

Maybe it's a lack of imagination for naming callback parameters, but it's a pattern I see being used relatively frequently.

Overall I don't really see the problem with shadowing variables with a function parameter.
Enabling the rule forces developer to come up with more names (which is hard!), something less explicit. And using less explicit name can easily create issues:

const love = get();
for (const loopLove of feelings) {
	// It's really easy here to use love instead of loopLove by mistake
}

Naming variables with meaningful names is more important than shadowing considerations.

What is important though is no-shadow-restricted-names which is enabled in XO.

@fregante
Copy link
Member Author

It's really easy here to use love instead of loopLove by mistake

If you do that, loopLove will be marked as unused. On the other hand with the current situation you can use love thinking it’s the outside love. In real life it’s usually less clear than this and maybe you set love inside a longer function where you want to use both.

I agree with the first part for .find and .reduce, it looks like this could be an exception for sync callbacks that follow the assignment.

@sindresorhus sindresorhus changed the title Add no-shadow Add no-shadow Jan 14, 2019
@sindresorhus sindresorhus transferred this issue from xojs/eslint-config-xo Jan 14, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants