Add prevent-abbreviations rule

[futpib]

Fixes #169

[sindresorhus]

I tried running this on the AVA codebase.

❯ xo api.js
can't resolve reference #/definitions/abbreviations from id #

❯ xo profile.js
can't resolve reference #/definitions/abbreviations from id #
AssertionError [ERR_ASSERTION]: Fix objects must not be overlapped in a report.
    at mergeFixes (/Users/sindresorhus/dev/oss/xo/node_modules/eslint/lib/util/report-translator.js:150:9)
    at normalizeFixes (/Users/sindresorhus/dev/oss/xo/node_modules/eslint/lib/util/report-translator.js:180:16)
    at args (/Users/sindresorhus/dev/oss/xo/node_modules/eslint/lib/util/report-translator.js:278:18)
    at Object.report (/Users/sindresorhus/dev/oss/xo/node_modules/eslint/lib/linter.js:720:41)
    at checkVariable (/Users/sindresorhus/dev/oss/eslint-plugin-unicorn/rules/prevent-abbreviations.js:192:11)
    at Array.forEach (<anonymous>)
    at checkVariables (/Users/sindresorhus/dev/oss/eslint-plugin-unicorn/rules/prevent-abbreviations.js:196:19)
    at checkScope (/Users/sindresorhus/dev/oss/eslint-plugin-unicorn/rules/prevent-abbreviations.js:204:3)
    at Array.forEach (<anonymous>)
    at checkChildScopes (/Users/sindresorhus/dev/oss/eslint-plugin-unicorn/rules/prevent-abbreviations.js:200:21)

[futpib]

I wonder why this did not come up in the integration tests run (the error itself is not fixed yet).

[futpib]

Fixed the integration test and the error.

@sindresorhus Should I cherry-pick the integration test fix into a separate PR?

[sindresorhus]

In AVA:

~/dev/oss/ava
❯ xo lib/fork.js --fix

  lib/fork.js:53:8
  ✖  53:8  Parsing error: Binding arguments in strict mode

  1 error

[sindresorhus]

In ~/dev/oss/ava/bench/run.js, it did the following auto-fix, which is not safe:

It should instead turn err, into err: error, and report about the err, property so I can manually fix it.

[sindresorhus]

I would recommend running xo --fix in the AVA project and then run npm run test:tap to ensure the tests still pass. That's a good way to validate the changes.

[sindresorhus]

Should we also report on properties (obviously not auto-fix them)? For example, foo.err should be foo.error.

Hmm, or maybe we should only report when creating objects and not using them?

const foo = {
	err: new Error();
};

[sindresorhus]

Instead of converting, for example, mod to module2, because module is taken, how do you feel about doing mod => module_?

[futpib]

Should we also report on properties (obviously not auto-fix them)? For example, foo.err should be foo.error.
Hmm, or maybe we should only report when creating objects and not using them?

Unlike variable, property names can be a part of an interface, so users may be forced to use certain property names by third party code. We could add another rule for property/method names and set it to warn.

Instead of converting, for example, mod to module2, because module is taken, how do you feel about doing mod => module_?

I prefer module_, and that's what I do usually write manually when renaming a variable just to prevent shadowing another variable.

What should the new general renaming scheme be?

• module_
• module__
• module___

Also, if we go with _s, we better update catch-error-name too for consistency

eslint-plugin-unicorn/test/catch-error-name.js

Lines 76 to 80 in 6f78698

	const error2 = new Error('foo bar');
	const error3 = new Error('foo bar');
	const error4 = new Error('foo bar');
	const error5 = new Error('foo bar');
	const error6 = new Error('foo bar');

[sindresorhus]

Unlike variable, property names can be a part of an interface, so users may be forced to use certain property names by third party code. We could add another rule for property/method names and set it to warn.

You could argue the same about the camelcase rule in ESLint though. The fix there is to use a disable-comment. I find warn pretty useless as it's too easy to ignore and won't be visible when run in a CI. How about we add an option of whether to check property/method names (true by default)? And obviously not auto-fix it.

[sindresorhus]

What should the new general renaming scheme be?

• module_
• module__
• module___

👍 I can't imagine more than one _ coming up in practice so seems fine.

Also, if we go with _s, we better update catch-error-name too for consistency

👍

[futpib]

I'm really not sure about any of this.

Default import does not seem much different from named import. This renaming is equally weird to me:

-import { prop } from 'ramda';
+import { prop as property } from 'ramda';

Which is analogous to an object destructuring

-const { prop } = require('ramda');
+const { prop: property } = require('ramda');

or

-const { prop } = foo;
+const { prop: property } = foo;

So I think if abbreviated default import is to be allowed, abbreviated named imports and variable names that are defined in object destructuring should be allowed too.

In case abbreviated property name came from the same project that is being linted, it will be reported wherever the object is created (or property assigned, or name exported).

I'm not sure which way it should be, but I think rules should be the same for all cases I mentioned (default import, named import and object destructuring).

[sindresorhus]

I agree it should be the same for all kinds of imports/requires.

Since we already have the logic for handling imports, it would be too bad to throw it away, but I also don't really think it's a good default. So how about we put it behind an option that is off by default?

[futpib]

I agree it should be the same for all kinds of imports/requires.

Do you agree about destructuring in general? That is that by default these should pass too:

const {prop} = foo;

const f = ({prop}) => prop;

[sindresorhus]

Yes, they should instead be renamed at the source, if possible.

[futpib]

@sindresorhus I'd like to change the rules around arguments to be the same as with taken variable names and reserved words. That is instead of allowing args, report it and rename it to arguments_. What do you think?

[sindresorhus]

That is instead of allowing args, report it and rename it to arguments_. What do you think?

👍

[futpib]

Done, once again 😆

[sindresorhus]

And now it's perfect! 👌

Thanks for your very hard work and perseverance on this one, @futpib. The result is fantastic.

[sindresorhus]

@futpib You have to submit the PR URL to https://issuehunt.io/r/sindresorhus/eslint-plugin-unicorn/issues/169 to claim the bounty ;)