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

[DO NOT MERGE] Auto fix prefer-destructuring-in-parameters on codebase #1046

Closed
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
30 commits
Select commit Hold shift + click to select a range
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Jump to
Jump to file
Failed to load files.
Diff view
Diff view
38 changes: 38 additions & 0 deletions docs/rules/prefer-destructuring-in-parameters.md
Original file line number Diff line number Diff line change
@@ -0,0 +1,38 @@
# Prefer destructuring in parameters over accessing properties

Makes your code shorter and nicer.

This rule is fixable.

## Fail

```js
const getObjectProperty = object => object.property;
```

```js
const removeEmptyValues = object => Object.fromEntries(
Object.entries(object).filter(keyValuePair => Boolean(keyValuePair[1]))
);
```

## Pass

```js
const getFoo = ({property}) => property;
```

```js
const removeEmptyValues = object => Object.fromEntries(
Object.entries(object).filter(([, value]) => Boolean(value))
);
```

```js
// Used property and index together
function foo(array) {
if (array.length > 0) {
return bar(array[0]);
}
}
```
1 change: 1 addition & 0 deletions index.js
Original file line number Diff line number Diff line change
Expand Up @@ -88,6 +88,7 @@ module.exports = {
'unicorn/prefer-array-some': 'error',
'unicorn/prefer-date-now': 'error',
'unicorn/prefer-default-parameters': 'error',
'unicorn/prefer-destructuring-in-parameters': 'error',
'unicorn/prefer-dom-node-append': 'error',
'unicorn/prefer-dom-node-dataset': 'error',
'unicorn/prefer-dom-node-remove': 'error',
Expand Down
2 changes: 2 additions & 0 deletions readme.md
Original file line number Diff line number Diff line change
Expand Up @@ -80,6 +80,7 @@ Configure it in `package.json`.
"unicorn/prefer-array-some": "error",
"unicorn/prefer-date-now": "error",
"unicorn/prefer-default-parameters": "error",
"unicorn/prefer-destructuring-in-parameters": "error",
"unicorn/prefer-dom-node-append": "error",
"unicorn/prefer-dom-node-dataset": "error",
"unicorn/prefer-dom-node-remove": "error",
Expand Down Expand Up @@ -158,6 +159,7 @@ Configure it in `package.json`.
- [prefer-array-some](docs/rules/prefer-array-some.md) - Prefer `.some(…)` over `.find(…)`.
- [prefer-date-now](docs/rules/prefer-date-now.md) - Prefer `Date.now()` to get the number of milliseconds since the Unix Epoch. *(fixable)*
- [prefer-default-parameters](docs/rules/prefer-default-parameters.md) - Prefer default parameters over reassignment. *(fixable)*
- [prefer-destructuring-in-parameters](docs/rules/prefer-destructuring-in-parameters.md) - Prefer destructuring in parameters over accessing properties. *(fixable)*
- [prefer-dom-node-append](docs/rules/prefer-dom-node-append.md) - Prefer `Node#append()` over `Node#appendChild()`. *(fixable)*
- [prefer-dom-node-dataset](docs/rules/prefer-dom-node-dataset.md) - Prefer using `.dataset` on DOM elements over `.setAttribute(…)`. *(fixable)*
- [prefer-dom-node-remove](docs/rules/prefer-dom-node-remove.md) - Prefer `childNode.remove()` over `parentNode.removeChild(childNode)`. *(fixable)*
Expand Down
12 changes: 6 additions & 6 deletions rules/consistent-destructuring.js
Original file line number Diff line number Diff line change
Expand Up @@ -86,10 +86,10 @@ const create = context => {
return;
}

const destructurings = objectPattern.properties.filter(property =>
property.type === 'Property' &&
property.key.type === 'Identifier' &&
property.value.type === 'Identifier'
const destructurings = objectPattern.properties.filter(({type, key, value}) =>
type === 'Property' &&
key.type === 'Identifier' &&
value.type === 'Identifier'
);
const lastProperty = objectPattern.properties[objectPattern.properties.length - 1];
const hasRest = lastProperty && lastProperty.type === 'RestElement';
Expand All @@ -98,8 +98,8 @@ const create = context => {
const member = source.getText(node.property);

// Member might already be destructured
const destructuredMember = destructurings.find(property =>
property.key.name === member
const destructuredMember = destructurings.find(({key}) =>
Copy link
Owner

Choose a reason for hiding this comment

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

The problem with destructuring is that you lose some readability information. It's no longer clear when you read this that key is the key of a property...

key.name === member
);

if (!destructuredMember) {
Expand Down
36 changes: 18 additions & 18 deletions rules/consistent-function-scoping.js
Original file line number Diff line number Diff line change
Expand Up @@ -28,8 +28,8 @@ function checkReferences(scope, parent, scopeManager) {
return isSameScope(parent, resolved.scope);
});

const hitDefinitions = definitions => definitions.some(definition => {
const scope = scopeManager.acquire(definition.node);
const hitDefinitions = definitions => definitions.some(({node}) => {
const scope = scopeManager.acquire(node);
return isSameScope(parent, scope);
});

Expand Down Expand Up @@ -70,10 +70,10 @@ function checkReferences(scope, parent, scopeManager) {
return getReferences(scope)
.map(({resolved}) => resolved)
.filter(Boolean)
.some(variable =>
hitReference(variable.references) ||
hitDefinitions(variable.defs) ||
hitIdentifier(variable.identifiers)
.some(({references, defs, identifiers}) =>
hitReference(references) ||
hitDefinitions(defs) ||
hitIdentifier(identifiers)
);
}

Expand All @@ -90,18 +90,18 @@ const reactHooks = new Set([
'useLayoutEffect',
'useDebugValue'
]);
const isReactHook = scope =>
scope.block &&
scope.block.parent &&
scope.block.parent.callee &&
scope.block.parent.callee.type === 'Identifier' &&
reactHooks.has(scope.block.parent.callee.name);

const isArrowFunctionWithThis = scope =>
scope.type === 'function' &&
scope.block &&
scope.block.type === 'ArrowFunctionExpression' &&
(scope.thisFound || scope.childScopes.some(scope => isArrowFunctionWithThis(scope)));
const isReactHook = ({block}) =>
Copy link
Owner

Choose a reason for hiding this comment

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

It's no longer possible to infer from the function what type it supports (that it supports a scope...

block &&
block.parent &&
block.parent.callee &&
block.parent.callee.type === 'Identifier' &&
reactHooks.has(block.parent.callee.name);

const isArrowFunctionWithThis = ({type, block, thisFound, childScopes}) =>
type === 'function' &&
block &&
block.type === 'ArrowFunctionExpression' &&
(thisFound || childScopes.some(scope => isArrowFunctionWithThis(scope)));
Comment on lines +100 to +104
Copy link
Owner

Choose a reason for hiding this comment

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

I think the previous version was more readable, and not much longer.


const iifeFunctionTypes = new Set([
'FunctionExpression',
Expand Down
36 changes: 18 additions & 18 deletions rules/custom-error-definition.js
Original file line number Diff line number Diff line change
Expand Up @@ -18,12 +18,12 @@ const getConstructorMethod = className => `
}
`;

const hasValidSuperClass = node => {
if (!node.superClass) {
const hasValidSuperClass = ({superClass}) => {
if (!superClass) {
return false;
}

let {name, type, property} = node.superClass;
let {name, type, property} = superClass;

if (type === 'MemberExpression') {
({name} = property);
Expand All @@ -32,20 +32,20 @@ const hasValidSuperClass = node => {
return nameRegexp.test(name);
};

const isSuperExpression = node =>
node.type === 'ExpressionStatement' &&
node.expression.type === 'CallExpression' &&
node.expression.callee.type === 'Super';
const isSuperExpression = ({type, expression}) =>
type === 'ExpressionStatement' &&
expression.type === 'CallExpression' &&
expression.callee.type === 'Super';

const isAssignmentExpression = (node, name) => {
const isAssignmentExpression = ({type, expression}, name) => {
if (
node.type !== 'ExpressionStatement' ||
node.expression.type !== 'AssignmentExpression'
type !== 'ExpressionStatement' ||
expression.type !== 'AssignmentExpression'
) {
return false;
}

const lhs = node.expression.left;
const lhs = expression.left;

if (!lhs.object || lhs.object.type !== 'ThisExpression') {
return false;
Expand Down Expand Up @@ -88,7 +88,7 @@ const customErrorDefinition = (context, node) => {
}

const {body, range} = node.body;
const constructor = body.find(x => x.kind === 'constructor');
const constructor = body.find(({kind}) => kind === 'constructor');

if (!constructor) {
context.report({
Expand Down Expand Up @@ -160,10 +160,10 @@ const customErrorDefinition = (context, node) => {
}
};

const customErrorExport = (context, node) => {
const exportsName = node.left.property.name;
const customErrorExport = (context, {left, right}) => {
const exportsName = left.property.name;

const maybeError = node.right;
const maybeError = right;

if (maybeError.type !== 'ClassExpression') {
return;
Expand All @@ -185,16 +185,16 @@ const customErrorExport = (context, node) => {
}

context.report({
node: node.left.property,
node: left.property,
messageId: MESSAGE_ID_INVALID_EXPORT,
fix: fixer => fixer.replaceText(node.left.property, errorName)
fix: fixer => fixer.replaceText(left.property, errorName)
});
};

const create = context => {
return {
ClassDeclaration: node => customErrorDefinition(context, node),
'AssignmentExpression[right.type="ClassExpression"]': node => customErrorDefinition(context, node.right),
'AssignmentExpression[right.type="ClassExpression"]': ({right}) => customErrorDefinition(context, right),
'AssignmentExpression[left.type="MemberExpression"][left.object.type="Identifier"][left.object.name="exports"]': node => customErrorExport(context, node)
};
};
Expand Down
4 changes: 2 additions & 2 deletions rules/empty-brace-spaces.js
Original file line number Diff line number Diff line change
Expand Up @@ -17,8 +17,8 @@ const selector = `:matches(${
const create = context => {
const sourceCode = context.getSourceCode();
return {
[selector](node) {
let [start, end] = node.range;
[selector]({range}) {
let [start, end] = range;
start += 1;
end -= 1;

Expand Down
44 changes: 22 additions & 22 deletions rules/expiring-todo-comments.js
Original file line number Diff line number Diff line change
Expand Up @@ -254,7 +254,7 @@ const create = context => {
const comments = sourceCode.getAllComments();
const unusedComments = flatten(
comments
.filter(token => token.type !== 'Shebang')
.filter(({type}) => type !== 'Shebang')
// Block comments come as one.
// Split for situations like this:
// /*
Expand Down Expand Up @@ -286,12 +286,12 @@ const create = context => {
};
const rules = baseRule.create(fakeContext);

function processComment(comment) {
if (ignoreRegexes.some(ignore => ignore.test(comment.value))) {
function processComment({value, loc}) {
if (ignoreRegexes.some(ignore => ignore.test(value))) {
return;
}

const parsed = parseTodoWithArguments(comment.value, options);
const parsed = parseTodoWithArguments(value, options);

if (!parsed) {
return true;
Expand All @@ -312,11 +312,11 @@ const create = context => {
if (dates.length > 1) {
uses++;
context.report({
loc: comment.loc,
loc: loc,
messageId: MESSAGE_ID_AVOID_MULTIPLE_DATES,
data: {
expirationDates: dates.join(', '),
message: parseTodoMessage(comment.value)
message: parseTodoMessage(value)
}
});
} else if (dates.length === 1) {
Expand All @@ -326,11 +326,11 @@ const create = context => {
const shouldIgnore = options.ignoreDatesOnPullRequests && ci.isPR;
if (!shouldIgnore && reachedDate(date)) {
context.report({
loc: comment.loc,
loc: loc,
messageId: MESSAGE_ID_EXPIRED_TODO,
data: {
expirationDate: date,
message: parseTodoMessage(comment.value)
message: parseTodoMessage(value)
}
});
}
Expand All @@ -339,13 +339,13 @@ const create = context => {
if (packageVersions.length > 1) {
uses++;
context.report({
loc: comment.loc,
loc: loc,
messageId: MESSAGE_ID_AVOID_MULTIPLE_PACKAGE_VERSIONS,
data: {
versions: packageVersions
.map(({condition, version}) => `${condition}${version}`)
.join(', '),
message: parseTodoMessage(comment.value)
message: parseTodoMessage(value)
}
});
} else if (packageVersions.length === 1) {
Expand All @@ -358,11 +358,11 @@ const create = context => {
const compare = semverComparisonForOperator(condition);
if (packageVersion && compare(packageVersion, decidedPackageVersion)) {
context.report({
loc: comment.loc,
loc: loc,
messageId: MESSAGE_ID_REACHED_PACKAGE_VERSION,
data: {
comparison: `${condition}${version}`,
message: parseTodoMessage(comment.value)
message: parseTodoMessage(value)
}
});
}
Expand All @@ -384,11 +384,11 @@ const create = context => {

if (trigger) {
context.report({
loc: comment.loc,
loc: loc,
messageId,
data: {
package: dependency.name,
message: parseTodoMessage(comment.value)
message: parseTodoMessage(value)
}
});
}
Expand All @@ -409,11 +409,11 @@ const create = context => {

if (compare(targetPackageVersion, todoVersion)) {
context.report({
loc: comment.loc,
loc: loc,
messageId: MESSAGE_ID_VERSION_MATCHES,
data: {
comparison: `${dependency.name} ${dependency.condition} ${dependency.version}`,
message: parseTodoMessage(comment.value)
message: parseTodoMessage(value)
}
});
}
Expand Down Expand Up @@ -441,11 +441,11 @@ const create = context => {

if (compare(targetPackageEngineVersion, todoEngine)) {
context.report({
loc: comment.loc,
loc: loc,
messageId: MESSAGE_ID_ENGINE_MATCHES,
data: {
comparison: `node${engine.condition}${engine.version}`,
message: parseTodoMessage(comment.value)
message: parseTodoMessage(value)
}
});
}
Expand All @@ -465,12 +465,12 @@ const create = context => {
if (parseArgument(testString).type !== 'unknowns') {
uses++;
context.report({
loc: comment.loc,
loc: loc,
messageId: MESSAGE_ID_MISSING_AT_SYMBOL,
data: {
original: unknown,
fix: testString,
message: parseTodoMessage(comment.value)
message: parseTodoMessage(value)
}
});
continue;
Expand All @@ -482,12 +482,12 @@ const create = context => {
if (parseArgument(withoutWhitespace).type !== 'unknowns') {
uses++;
context.report({
loc: comment.loc,
loc: loc,
messageId: MESSAGE_ID_REMOVE_WHITESPACE,
data: {
original: unknown,
fix: withoutWhitespace,
message: parseTodoMessage(comment.value)
message: parseTodoMessage(value)
}
});
continue;
Expand Down