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

Use singular array element variable name in autofix for no-for-loop rule #745

Merged
merged 6 commits into from
May 27, 2020

Conversation

bmish
Copy link
Contributor

@bmish bmish commented May 24, 2020

If the loop is iterating an array with a plural name (such as plugins), the generated element variable in the autofix could use the singular version of the array name (plugin), instead of just the generic name element.

Fixes #743. CC: @mongoose700

@bmish
Copy link
Contributor Author

bmish commented May 24, 2020

@fisker just updated to avoid using reserved JavaScript keywords.

@fisker
Copy link
Collaborator

fisker commented May 24, 2020

for (const case_ of cases) {}
for (const element of cases) {}

Which one do you prefer?

@bmish
Copy link
Contributor Author

bmish commented May 24, 2020

@fisker no strong preference, either works for me.

@fisker
Copy link
Collaborator

fisker commented May 25, 2020

I prefer

for (const case_ of cases) {}

over

for (const element of cases) {}

And

for (const function_ of functions) {}

over

for (const element of functions) {}

If we decide to use case_, we need fix #747 first, then we can use avoidCapture to change case to case_

@sindresorhus
Copy link
Owner

I also prefer case_.

@snowteamer
Copy link

I kinda dislike both of them, since I think one should try not using identifiers like cases or functions in the first place.

That being said, I also prefer the case_ form, because I think the trailing underscore may serve as a nice visual reminder to the user that she should think about renaming the variable.

@bmish bmish force-pushed the no-for-loop-singular branch 2 times, most recently from 4914cc1 to 9fa0424 Compare May 27, 2020 02:43
@bmish
Copy link
Contributor Author

bmish commented May 27, 2020

Rebased to take advantage of the avoidCapture improvement from #749 for producing the case_ variable name.

…-loop` rule

If the loop is iterating an array with a plural name (such as `plugins`), the generated element variable in the autofix could use the singular version of the array name (`plugin`), instead of just the generic name `element`.
@fisker
Copy link
Collaborator

fisker commented May 27, 2020

invalid: [
	...[
	  ['plugin', 'plugins'],
	  ['person', 'people'],
	  ['element', 'list'],
	].map(([elementName, arrayName]) => 
	  testCase(
	    `for(const i = 0; i < ${arrayName}.length; i++) {console.log(${arrayName}[i])}`,
	    `for(const ${elementName} of ${arrayName}) {console.log(${elementName})}`,
	  )
	),
]

Can you rewrite tests to this? So we can test more cases easier.

@fisker
Copy link
Collaborator

fisker commented May 27, 2020

I ask you again, don't squash!

@bmish
Copy link
Contributor Author

bmish commented May 27, 2020

@fisker good idea, updated test cases. Sorry, squashing is a habit, I will try to avoid it.

@fisker
Copy link
Collaborator

fisker commented May 27, 2020

Add some words ends with s? Even if it was wrong.
Also add one test that uses i.

@fisker
Copy link
Collaborator

fisker commented May 27, 2020

Add a word that has more than one plural, girlsAndBoys?

rules/no-for-loop.js Outdated Show resolved Hide resolved
bmish and others added 2 commits May 26, 2020 22:33
Co-authored-by: fisker Cheung <lionkay@gmail.com>
Copy link
Collaborator

@fisker fisker left a comment

Choose a reason for hiding this comment

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

Looks good.

@sindresorhus sindresorhus merged commit 2002093 into sindresorhus:master May 27, 2020
@sindresorhus
Copy link
Owner

Nice work :)

@fisker
Copy link
Collaborator

fisker commented May 27, 2020

@bmish Do you interested in improving no-fn-reference-in-iterator rule? We use element there too. No pressure.

@bmish
Copy link
Contributor Author

bmish commented May 27, 2020

@fisker I'll see if I have a chance sometime, thanks for the idea. Can you file a ticket for now?

@fisker
Copy link
Collaborator

fisker commented May 28, 2020

@bmish #754

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

no-for-loop rule could use smarter element variable name in autofix
4 participants