reactioncommerce / reaction-eslint-config

Reaction Eslint Configuration
MIT License
5 stars 6 forks source link

add `i` to id-length exception #10

Closed kieckhafer closed 5 years ago

kieckhafer commented 5 years ago

I'm proposing to add i to the exceptions for the id-length rule.

The first example from the es-lint docs states:

var x = 5; // too short; difficult to understand its purpose without context

Based on this description, I believe that i is well enough known as an incrementer, and therefore should be an exception as the purpose is known without other context.

for (let i = 0; i < appsOrder.length; i += 1) {

https://github.com/reactioncommerce/reaction/issues/5268

brent-hoover commented 5 years ago

No one asked me but my vote would be no because while i is a well-known incrementor, nothing in eslint prevents you from using i in other cases so you will miss improperly used single character variables in exchange for a very small amount of time saved.

focusaurus commented 5 years ago

I'm kinda meh about this. I prefer for..of statements mdn docs and .forEach() calls. I essentially never write for loops anymore. I'm OK with this config change but would like to also see us refactor toward those alternative syntaxes.

kieckhafer commented 5 years ago

@focusaurus @zenweasel Appreciate both the inputs.

@zenweasel makes sense completely. I audited the way we use i, and we seem to always use it in this way, however, that might not always be the case, and you're right, it probably doesn't make sense then to update this just in case.

@focusaurus Let's leave it as it is, also agree that I don't use for as much anymore, we should move towards the new syntaxes, so we can just leave it as it is since we won't be using the for loop and then i might actually be a short out of context var. I'll close this PR.

Thanks!

brent-hoover commented 5 years ago

Also, if we have a style choice like prefering .forEach or whatever, let's document that somewhere. I think we have a code convention doc in the contributor guide. Maybe there's even a eslint rule we could add for that?