Prefer non-abbreviated variable names

[fregante]

The ESLint rule suggests:

Using complete words results in more readable code. Not everyone knows all your abbreviations. Code is written only once, but read many times.

This PR is just the result of an autofix and is only meant to demo its results. I had already enabled it a long while ago avoid e and err on errors, but the rule actually includes more words.

What do you think? Some abbreviations can still be allowed, for example Props since it's what React uses. Other cases like fn and args take some mental adjustment if not excluded since they end up as function_ and arguments_ respectively.

[twschiller]

What do you think? Some abbreviations can still be allowed, for example Props since it's what React uses. Other cases like fn and args take some mental adjustment if not excluded since they end up as function_ and arguments_ respectively.

I'm generally in favor. As you mention, there are some social norms (community specific, or for practical reasons) that we'd want to exclude from the rule:

Allowable:

• args
• props

Perhaps oddly I like "num". (Because to me it means more count/size as compared to an actually number)

@BLoe @BALEHOK @mnholtz any thoughts on particular abbreviations?

[fregante]

I reduced the scope, this is ready to merge (being a lengthy lint PR, sooner rather than later)

[fregante]

I ended up allowing a few abbreviations either because they're used by React or because their replacements aren't ideal (args -> arguments_ 🙅‍♂️)

[fregante]

We almost never need to use simple for loops. This function could be replaced by

return new Uint8Array(binary.map(c => c.charCodeAt(0)))

and in general for-or coupled with Object.entries() or [].entries() has better ergonomics

[BLoe]

There's also an existing lint rule that prompts you to use for-of in a lot of cases.

[BLoe]

It doesn't bother me enough to really protest against this if other people are in favor of it, but the whole idea of this kind of rubs me the wrong way. All of these are super commonly used programming abbreviations, e.g. var, attr, obj, i, prev, etc.

The bad kind of names are ones where it isn't clear what the abbreviation means, which are random one-off cases that a linter like this obviously wouldn't be able to catch. So what this is saying is that you can't use common patterns that every developer uses and understands, but it won't fail if you use bad abbreviations. Just seems like the wrong incentives here and fixing for the wrong thing. Like, how much time is it going to cost me over the lifetime of this codebase having to write "ious" in the right order after "prev" compared to the amount of time I would spend remembering what "prev" means while reading code? Doesn't seem like a great ROI to me.

[fregante]

I had the same knee-jerk reaction to some of these, but that's just what it is.

it won't fail if you use bad abbreviations

Jst cos it cant cach al bad cod it dont mean we should give up entirly. We can read dis but dat dosnt make it gud.

e is also a common variable name and this PR fixes that. Is that event, error or element? Should we leave it because that's how it's always been done?

Even obj is understandable but it doesn't mean it's a good name, in fact the rule suggests fixing it to something more specific than that.

how much time is it going to cost me over the lifetime of this codebase having to write "ious" in the right order

I see what you're saying, but the amount of time it takes to type 4 characters isn't much at all. It feels weird to talk about ROI for that. After the first usage it will be autocompleted. If mistyped, all variables can be renamed with a single shortcut. Even if your editor autocompletes the wrong thing you're one auto-fix away from using readable variable names.

[fregante]

The last point is the most important one: We can continue using prev and have the editor autofix that on save or before committing. In practice this shouldn't really affect us while writing code.

[BLoe]

I had the same knee-jerk reaction to some of these, but that's just what it is.

it won't fail if you use bad abbreviations

Jst cos it cant cach al bad cod it dont mean we should give up entirly. We can read dis but dat dosnt make it gud.

I'm not sure I understand your point here. I was trying to say that it catches code that isn't that bad, and doesn't catch code that is actually really bad.

e is also a common variable name and this PR fixes that. Is that event, error or element?

In my (limited) experience, using e for HTML event handlers is very standard, and I've never seen it regularly used to abbreviate error or element or anything else.

I see what you're saying, but the amount of time it takes to type 4 characters isn't much at all. It feels weird to talk about ROI for that.

But that's more than the time it takes to not type those 4 characters, which is zero time. I think discussing developer time spent on things is crucial for a startup with limited resources like ours.

We can continue using prev and have the editor autofix that on save or before committing

I suppose this is the main point here that would convince me that this is ok. If it's just like running prettier in pre-commit or whatever, then I'm generally ok with it.

My main concern is simply "What is the cost to the business if we don't implement this, versus what is the cost we will pay in overhead if we do add this?" Even small things like this, on aggregate, can add up to a lot of developer time one way or the other over the lifetime of the company.

[mnholtz]

I think I'm generally on board with enforcing naming conventions that allow the code to read more like an actual english sentence. I also think adding the lint rules will enforce consistency, which is probably a good thing. I'm happy with the current list of common developer abbreviation exceptions.

Although, I'm not sure how I feel about num. I think even number is kind of a poor name for something (although this obviously would not get caught in this new lint rule), why not just use a more descriptive count, size, or index if num is really being used for this purpose? I'm happy with it being in the list though. You just might not see me using it very often :P

In my (limited) experience, using e for HTML event handlers is very standard, and I've never seen it regularly used to abbreviate error or element or anything else.

In my experience, I have definitely seen e used for all of these things before. Perhaps more often in vanilla javascript, though.

[mnholtz]

Similarly to the string_ comment above, not a fan of the trailing underscore. Could this be permissionsOptions instead?

[twschiller]

• Avoid trailing underscores

[fregante]

I see what happened here: There's an existing variable called options in the parameters, hence the trailing underscore. opts was just used as an alternative name for option, but both alternatives fall into the "not specific enough" category.

Thankfully this is avoidable by just using the object desctructuring fully.

[twschiller]

I'm by and large OK with these changes. Added todos where there were spots team members

My main complaint is I prefer "obj" to "object". "object" is hard to read since it's a type in Typescript. Just like how we have "str" vs. string

• Consider allowing obj vs. object to distinguish it from the class Object and type object

My main concern is simply "What is the cost to the business if we don't implement this, versus what is the cost we will pay in overhead if we do add this?" Even small things like this, on aggregate, can add up to a lot of developer time one way or the other over the lifetime of the company.

My preference is toward standardization with automated tool support. A key part of this is adapting the standards/tools to our particular project and preferences.

Code is written once and read hundreds of times by different people. So by aligning on reasonable standards we can speed up future changes and PR reviews (and reduce onboarding time)

[twschiller]

Add @BALEHOK because this impacts all team members

See my comment about allowing "str" and "obj" to distinguish them from the classes/types

[fregante]

My main complaint is I prefer "obj" to "object". "object" is hard to read since it's a type in Typescript. Just like how we have "str" vs. string

I think both of them fall into the initial reasoning of this rule though. I don't see how them matching the type would warrant an exclusion unless we block variables named number symbol string object. The object type specifically is not advised by another rule

The count of exclusions being suggested amounts to fixing just msg e len val and arr — which is fine. We can be honest about not liking long variable names and just enforce a limited subset, I'm not emotionally invested in this 😁

I can fix some of the autofix mistakes here or I can change the config to a 6-word-long list of blocked abbreviations.

[twschiller]

I think both of them fall into the initial reasoning of this rule though. I don't see how them matching the type would warrant an exclusion unless we block variables named number symbol string object. The object type specifically is not advised by another rule

I don't think we should use any of those as variable/argument names. They're like "shadowing" an outer global. It's bad because the same name have two different meanings depending on where you look. And for quick reading, it's very dependent on syntax highlighting to see their role). object is only coincidentally banned because the semantics of it in typescript aren't good

The count of exclusions being suggested amounts to fixing just msg e len val and arr — which is fine. We can be honest about not liking long variable names and just enforce a limited subset

The feedback people have been giving is that there's a reason the abbreviations exists (beyond them just being shorter)

I'm in favor of enabling the rule for the less controversial ones and then we can adapt from there

[fregante]

I'll add one thing: The rule seems one-dimensional in that it just avoids abbreviations. In practice though this PR showed me many bad patterns that these abbreviations allowed, like:

const arr = [...array]
const opts = {
	default: 'something',
	...options
}

How are arr and opts different from array and options? That's the issue being highlighted here, even if indirectly. The linter’s intent is to use descriptive variable names.

Example solution: don't create an intermediary options object a6bed49

I'm in favor of enabling the rule for the less controversial ones and then we can adapt from there

Will do

[twschiller]

How are arr and opts different from array and options? That's the issue being highlighted here, even if indirectly. The linter’s intent is to use descriptive variable names.

I think that's a good point -- that abbreviations aren't an acceptable way to avoid giving things a better name. I wonder if there's a rule that catches use of the abbreviated form and non-abbreviated form within the same scope (arr and array and options and opts)? In the meantime, we can also try to be more diligent for catching these during PR review

Will do

Thanks!

[fregante]

Done 🎉

[fregante]

This has a trailing underscore because it avoids shadowing the native length global.

If desired, I can replace this piece of code with the suggested .map-based version, but I'd rather keep this PR related to formatting-only.

[BALEHOK]

Add @BALEHOK because this impacts all team members

To be honest I'm not a fan of strict linter rules. This one, in particular, seems kind of rigid to me and maybe we are over-complicating things here.
However, if the team finds this beneficial, I'm ok with the changes and the list of exceptions.

[twschiller]

Let's give this a try with the exclusions. If people hate it in practice (for both writing code and PR reviews), we'll be quick to turn off the rule