-
-
Notifications
You must be signed in to change notification settings - Fork 381
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
Rule proposal: Prevent abbreviations #169
Comments
Honestly, I think they're all quite controversial. I agree with you that that in general abbreviations are not good variable names, but I personally prefer the shorter version of most of these and I think they're pretty widely understood. I'd say Also, how reliable will this be? What if |
But is it really worth sacrificing readability over saving a few characters? Swift has taught me a lot about writing readable code. Some would say it's too verbose, but I also think JS is generally too concise. I know we have conventions and all that, but should we maybe stop and question those? I feel like everyone just does it because everyone else does it, without actually considering whether it's worth the downsides.
Neither the Node.js API nor docs are known for their approachability or user-friendliness, though.
I didn't intend for it to have auto-fixing, but if it did, it could only fix the non-ambiguous cases. |
I've also seen |
|
|
|
Abbreviations like Hence, I have no problem with |
Are they though? Only if you know that
I agree, |
If you, for example, disagree with |
It could, if you're building an RPG. In the general case I think that those abbreviations are common enough that (e.g.)
This is the one caveat I should've added to my original comment, but I was growing tired of typing. :) |
I personally also base naming decisions on scope size. While having a So as a rule of thumb, I’d say the larger the scope the more descriptive the variable names should be. |
The following ones came to my mind spontaneously:
|
|
Are we also considering shorthands like
I see this a lot more in more academic uses, though it does happen in the real world as well. |
arg => argument |
|
I agree with avoiding abbreviations in general, but I think the problem with this rule is that it will forbid all the abbreviations that are okay because they are widely understood (e.g. So I think a rule like this instead of blacklisting abbreviations, should for example check variable names against an English dictionary and/or enforce a minimum character length. |
I think |
Not in arrow functions, which I personally use most of the time. |
I agree the main focus should be to reduce ambiguity, the other cases could potentially be opt-in. |
That's fair, but then you have to ask yourself if you want consistency, too. |
@therealklanni If I had to choose, I would pick readability over consistency. |
I think this has the potential to be the worst of all possible bikesheds, and you'll hear from three groups of people:
If the goal is to ship this rule with a recommended set of abbreviations to avoid, my guess is that you won't find a consensus for more than a tiny handful of identifiers. |
@savetheclocktower This is just exploratory right now. I'm happy to hear all opinions. The main focus is the ambiguity aspect, which I don't think many would object to. I've personally been using the non-abbreviations in my code for a while now. I'm liking it so far, but it takes some getting used to. |
|
I use these pretty often:
|
|
I'm not sure why I feel uncomfortable with those expanded terms replacing all those rather well-known abbrevations, but I think I've come up with a possible explanation.
|
That's what minifiers are for (and character count is largely irrelevant for server-side) |
I see the following sometimes: ns -> namespace |
I don’t want to derail too much so I’m just going to link to the Nomenclature section of the Acronym article from Wikipedia. In short, one may draw a line between initialisms and acronyms but it’s not a clear-cut thing. In any case, I think the meaning behind my argument was clear. |
I'd say what privileges There's probably no hard and fast rule — I'd say, if you have two choices, you'll go with the one that appears more comprehensible for a decently tech-savvy non-programmer. However this seems incredibly hard to decide for an algorithm/linter... |
@sonicdoe: sorry, I try not to let pedantry slip in; I failed there. Still, my main point is simply "How do we draw the lines?" If we're working on a calendaring application, @loilo: that difficulty for an algorithm is what concerns me here. |
Understandable. However, a non-exhaustive |
I think we can all agree that this problem is not fully machine-solvable with current technology, so a curated blacklist is probably the only viable approach. That said, we should discuss way less about whether individual terms should be blacklisted, but rather try to agree on a deciding question to "ask" for those terms — roughly like this, but probably still more precise:
This still omits aspects like proportionality (we probably should not use a comprehensible 100 character word for an identifier even if it's more comprehensible than a 5 character term) etc., but I guess we just cannot leave out common sense from this whole topic. That said, if the declared question is not clearly and easily answered, then we got to the point where we'd have to draw a line. So, I think your concerns are valid @CrossEye, but if we can find a reasonable, standardized approach to blacklisting terms, those concerncs should very much be the exception rather than the rule. |
Just to be clear, the point is not to enforce this for all acronyms/abbreviations. The rule should only enforce this list: #169 (comment) Additional entries can be discussion in new issues later on. Everything in that list will be enabled by default and people can disable individual words if they want, like: {
words: {
err: false
}
} It should also be possible to set your own words and replacements. It should be possible to choose, both for the built-in and custom words, on individual words, whether the word should be auto-fixable or not. If there are multiple replacements, auto-fix will not run, and the error message will include all the possible words, for example, If the user tries to enable auto-fix for a word with multiple replacements, it should be an error. The list of words should be added directly to this plugin and not be in a separate module for now, so we can iterate faster. |
I'd like to implement this rule (even though I'll probably end up disabling it). What should the rule do if the preferred identifier is already taken? |
Report the error but don't auto-fix. Unless, you can think of something smarter? |
I would also like to see an additional option that let's users disable all the built-in words by default ( |
The only other options I have in mind are
|
This would actually be nice. The less I have to fix manually the better. I doubt it will come up that much anyway. Why would anyone use both an acronym and its long-form in the same scope for different variables? Could happen, but I can't imagine it's that common. |
While coming up with tests, some expected awkward interactions between built-in Currently my plan is to make the rule prevent the collision with the special @sindresorhus please take a look at the WIP tests, maybe we should instead allow the |
Yeah, I don't see a good way around this. Let's allow |
I took a freedom to rethink the concept a bit to make the rule more general and usable by more people. It should also help testing various rules before finalizing default rule sets. Initial rules implemented in I also used the name |
@stroncium There's already an open PR for this => #237 |
@sindresorhus Well, it wasn't put in submitted, and by the time I realized there was a PR, I've had it done already. Anyhow, perhaps you will find my version more useful. If not than so shall it be. |
Fixes #169 Co-authored-by: Sindre Sorhus <[email protected]>
@sindresorhus has rewarded $144.00 to @futpib. See it on IssueHunt
|
This rule is now available: https://github.com/sindresorhus/eslint-plugin-unicorn/blob/master/docs/rules/prevent-abbreviations.md |
It is a problem with ES6 classes, though. constructor (...args) {
super(...args);
this.protocol = 'http:';
this.defaultPort = 80;
} is raising a "unicorn/prevent-abbreviations" warning. |
constructor (...arguments_) {
super(...arguments_);
this.protocol = 'http:';
this.defaultPort = 80;
} |
@sindresorhus It is much more common to use |
I think @sindresorhus suggestion is valid given what this rule is enforcing. I have just added an exception using |
Sorry, you should configure "unicorn/prevent-abbreviations": [
2,
{
"replacements": {
"args": false,
"props": false
},
}
] |
Using complete words result in more readable code. Not everyone knows all your abbreviations. You only write code once, but it's read many times.
For a start, we could go with these:
err
=>error
cb
=>callback
opts
=>options
str
=>string
obj
=>object
num
=>number
val
=>value
e
=>event
/error
evt
=>event
el
=>element
req
=>request
res
=>response
/result
btn
=>button
msg
=>message
len
=>length
env
=>environment
dev
=>development
prod
=>production
tmp
=>temporary
arg
=>argument
args
=>arguments
tbl
=>table
db
=>database
ctx
=>context
mod
=>module
Also
i
=>index
, but I have a feeling, that it's going to be too controversial.Suggestions welcome for additional ones. Also looking for people's thoughts on this rule.
The text was updated successfully, but these errors were encountered: