Skip to content

Run sindresorhus#1221 on codebase #282

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.

Sign up for GitHub

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

Open
wants to merge 2 commits into
base: prefer-ternary-improve
Choose a base branch
from
Open

Run sindresorhus#1221 on codebase #282

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
8875576
Run https://github.com/sindresorhus/eslint-plugin-unicorn/pull/1221 o…
fisker Apr 27, 2021
8352eb8
Run `xo`
fisker Apr 27, 2021
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
24 changes: 4 additions & 20 deletions rules/consistent-function-scoping.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -21,11 +21,7 @@ function checkReferences(scope, parent, scopeManager) {
const [definition] = resolved.defs;

// Skip recursive function name
if (definition && definition.type === 'FunctionName' && resolved.name === definition.name.name) {
return false;
}

return isSameScope(parent, resolved.scope);
return definition && definition.type === 'FunctionName' && resolved.name === definition.name.name ? false : isSameScope(parent, resolved.scope);
Copy link

Choose a reason for hiding this comment

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

Either this should be excluded due its length, or:

Suggested change
return definition && definition.type === 'FunctionName' && resolved.name === definition.name.name ? false : isSameScope(parent, resolved.scope);
const condition = definition && definition.type === 'FunctionName' && resolved.name === definition.name.name;
return condition ? false : isSameScope(parent, resolved.scope);

Copy link

@fregante fregante Apr 27, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

I think it shouldn't use the ternary at all:

Suggested change
return definition && definition.type === 'FunctionName' && resolved.name === definition.name.name ? false : isSameScope(parent, resolved.scope);
return !(definition && definition.type === 'FunctionName' && resolved.name === definition.name.name) && isSameScope(parent, resolved.scope);

But it's still not readable as is. Maybe this (I'm not sure if the boolean logic is identical though):

Suggested change
return definition && definition.type === 'FunctionName' && resolved.name === definition.name.name ? false : isSameScope(parent, resolved.scope);
return !(
definition &&
definition.type === 'FunctionName' &&
resolved.name === definition.name.name
) && isSameScope(parent, resolved.scope);

But given the length, once again, it's probably not worth the change

});

const hitDefinitions = definitions => definitions.some(definition => {
Expand Down Expand Up @@ -58,13 +54,7 @@ function checkReferences(scope, parent, scopeManager) {
}

// Ignore identifiers from our own scope
if (isSameScope(scope, identifierParentScope)) {
return false;
}

// Look at the scope above the function definition to see if lives
// next to the reference being checked
return isSameScope(parent, identifierParentScope.upper);
return isSameScope(scope, identifierParentScope) ? false : isSameScope(parent, identifierParentScope.upper);
});

return getReferences(scope)
Expand Down Expand Up @@ -138,16 +128,10 @@ function checkNode(node, scopeManager) {
}

const parentScope = scopeManager.acquire(parentNode);
if (
!parentScope ||
return !parentScope ||
parentScope.type === 'global' ||
isReactHook(parentScope) ||
isIife(parentNode)
) {
return true;
}

return checkReferences(scope, parentScope, scopeManager);
isIife(parentNode) ? true : checkReferences(scope, parentScope, scopeManager);
}

const create = context => {
Expand Down
12 changes: 2 additions & 10 deletions rules/custom-error-definition.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -47,11 +47,7 @@ const isAssignmentExpression = (node, name) => {

const lhs = node.expression.left;

if (!lhs.object || lhs.object.type !== 'ThisExpression') {
return false;
}

return lhs.property.name === name;
return !lhs.object || lhs.object.type !== 'ThisExpression' ? false : lhs.property.name === name;
};

const isPropertyDefinition = (node, name) => {
Expand All @@ -64,11 +60,7 @@ const isPropertyDefinition = (node, name) => {
return false;
}

if (key.type !== 'Identifier') {
return false;
}

return key.name === name;
return key.type !== 'Identifier' ? false : key.name === name;
};

const customErrorDefinition = (context, node) => {
Expand Down
6 changes: 1 addition & 5 deletions rules/expiring-todo-comments.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -173,11 +173,7 @@ function parseTodoMessage(todoString) {
// Check if have to skip colon
// @example "TODO [...]: message here"
const dropColon = afterArguments[0] === ':';
if (dropColon) {
return afterArguments.slice(1).trim();
}

return afterArguments;
return dropColon ? afterArguments.slice(1).trim() : afterArguments;
Copy link

Choose a reason for hiding this comment

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

This is good

}

function reachedDate(past) {
Expand Down
11 changes: 2 additions & 9 deletions rules/explicit-length-check.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -79,9 +79,7 @@ function getLengthCheckNode(node) {
}

// Non-Zero length check
if (
// `foo.length !== 0`
isCompareRight(node, '!==', 0) ||
return isCompareRight(node, '!==', 0) ||
// `foo.length != 0`
isCompareRight(node, '!=', 0) ||
// `foo.length > 0`
Expand All @@ -95,12 +93,7 @@ function getLengthCheckNode(node) {
// `0 < foo.length`
isCompareLeft(node, '<', 0) ||
// `1 <= foo.length`
isCompareLeft(node, '<=', 1)
) {
return {isZeroLengthCheck: false, node};
}

return {};
isCompareLeft(node, '<=', 1) ? {isZeroLengthCheck: false, node} : {};
Copy link

Choose a reason for hiding this comment

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

This is fine

}

function create(context) {
Expand Down
18 changes: 3 additions & 15 deletions rules/filename-case.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -133,31 +133,19 @@ function englishishJoinWords(words) {
return words[0];
}

if (words.length === 2) {
return `${words[0]} or ${words[1]}`;
}

return `${words.slice(0, -1).join(', ')}, or ${words[words.length - 1]}`;
return words.length === 2 ? `${words[0]} or ${words[1]}` : `${words.slice(0, -1).join(', ')}, or ${words[words.length - 1]}`;
Copy link

Choose a reason for hiding this comment

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

This is fine, although maybe on 3 lines

Suggested change
return words.length === 2 ? `${words[0]} or ${words[1]}` : `${words.slice(0, -1).join(', ')}, or ${words[words.length - 1]}`;
return words.length === 2 ?
`${words[0]} or ${words[1]}` :
`${words.slice(0, -1).join(', ')}, or ${words[words.length - 1]}`;

Copy link
Owner Author

Choose a reason for hiding this comment

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

But the old one still better

function englishishJoinWords(words) {
	if (words.length === 1) {
		return words[0];
	}

	if (words.length === 2) {
		return `${words[0]} or ${words[1]}`;
	}

	return `${words.slice(0, -1).join(', ')}, or ${words[words.length - 1]}`;
}

Copy link

Choose a reason for hiding this comment

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

Indeed

Copy link
Owner Author

Choose a reason for hiding this comment

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

Problem is hard to detect this. because maybe some other stuff above words.length === 2

}

const create = context => {
const options = context.options[0] || {};
const chosenCases = getChosenCases(options);
const ignore = (options.ignore || []).map(item => {
if (item instanceof RegExp) {
return item;
}

return new RegExp(item, 'u');
return item instanceof RegExp ? item : new RegExp(item, 'u');
Copy link

Choose a reason for hiding this comment

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

This is fine

});
const chosenCasesFunctions = chosenCases.map(case_ => ignoreNumbers(cases[case_].fn));
const filenameWithExtension = context.getFilename();

if (filenameWithExtension === '<input>' || filenameWithExtension === '<text>') {
return {};
}

return {
return filenameWithExtension === '<input>' || filenameWithExtension === '<text>' ? {} : {
Copy link

Choose a reason for hiding this comment

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

This is ok, it was more readable before

Program: node => {
const extension = path.extname(filenameWithExtension);
const filename = path.basename(filenameWithExtension, extension);
Expand Down
6 changes: 1 addition & 5 deletions rules/import-style.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -112,11 +112,7 @@ const joinOr = words => {
return word;
}

if (index === (words.length - 2)) {
return word + ' or';
}

return word + ',';
return index === (words.length - 2) ? word + ' or' : word + ',';
})
.join(' ');
};
Expand Down
12 changes: 2 additions & 10 deletions rules/no-array-for-each.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -148,11 +148,7 @@ function getFixFunction(callExpression, sourceCode, functionInfo) {
return false;
}

if (callback.body.type !== 'BlockStatement') {
return false;
}

return true;
return callback.body.type === 'BlockStatement';
Copy link
Owner Author

Choose a reason for hiding this comment

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

@fregante This is what you want 😄

};

function * removeCallbackParentheses(fixer) {
Expand Down Expand Up @@ -311,11 +307,7 @@ function isFixable(callExpression, sourceCode, {scope, functionInfo, allIdentifi
return false;
}

if (isFunctionSelfUsedInside(callback, callbackScope)) {
return false;
}

return true;
return !isFunctionSelfUsedInside(callback, callbackScope);
}

const ignoredObjects = [
Expand Down
60 changes: 13 additions & 47 deletions rules/no-for-loop.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,11 +36,7 @@ const getIndexIdentifierName = forStatement => {
return;
}

if (variableDeclarator.id.type !== 'Identifier') {
return;
}

return variableDeclarator.id.name;
return variableDeclarator.id.type !== 'Identifier' ? undefined : variableDeclarator.id.name;
};

const getStrictComparisonOperands = binaryExpression => {
Expand Down Expand Up @@ -83,30 +79,18 @@ const getArrayIdentifierNameFromBinaryExpression = (binaryExpression, indexIdent
return;
}

if (greater.property.name !== 'length') {
return;
}

return greater.object.name;
return greater.property.name !== 'length' ? undefined : greater.object.name;
Copy link

@fregante fregante Apr 27, 2021 •
edited
Loading

Choose a reason for hiding this comment

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

It looks like this whole function could be merged into 2 return statements, but indeed the readability could potentially be trash, especially if there are comments explaining each step.

};

const getArrayIdentifierName = (forStatement, indexIdentifierName) => {
const {test} = forStatement;

if (!test || test.type !== 'BinaryExpression') {
return;
}

return getArrayIdentifierNameFromBinaryExpression(test, indexIdentifierName);
return !test || test.type !== 'BinaryExpression' ? undefined : getArrayIdentifierNameFromBinaryExpression(test, indexIdentifierName);
};

const isLiteralOnePlusIdentifierWithName = (node, identifierName) => {
if (node && node.type === 'BinaryExpression' && node.operator === '+') {
return (isIdentifierWithName(node.left, identifierName) && isLiteralOne(node.right)) ||
(isIdentifierWithName(node.right, identifierName) && isLiteralOne(node.left));
}

return false;
return node && node.type === 'BinaryExpression' && node.operator === '+' ? (isIdentifierWithName(node.left, identifierName) && isLiteralOne(node.right)) ||
(isIdentifierWithName(node.right, identifierName) && isLiteralOne(node.left)) : false;
};

const checkUpdateExpression = (forStatement, indexIdentifierName) => {
Expand Down Expand Up @@ -148,14 +132,8 @@ const isOnlyArrayOfIndexVariableRead = (arrayReferences, indexIdentifierName) =>
return false;
}

if (
node.parent.type === 'AssignmentExpression' &&
node.parent.left === node
) {
return false;
}

return true;
return !(node.parent.type === 'AssignmentExpression' &&
node.parent.left === node);
});
};

Expand All @@ -176,14 +154,10 @@ const getRemovalRange = (node, sourceCode) => {

const index = declarationNode.declarations.indexOf(node);

if (index === 0) {
return [
node.range[0],
declarationNode.declarations[1].range[0]
];
}

return [
return index === 0 ? [
node.range[0],
declarationNode.declarations[1].range[0]
] : [
declarationNode.declarations[index - 1].range[1],
node.range[1]
];
Expand Down Expand Up @@ -235,11 +209,7 @@ const isIndexVariableUsedElsewhereInTheLoopBody = (indexVariable, bodyScope, arr
return true;
}

if (node.object.name !== arrayIdentifierName) {
return true;
}

return false;
return node.object.name !== arrayIdentifierName;
});

return referencesOtherThanArrayAccess.length > 0;
Expand Down Expand Up @@ -343,11 +313,7 @@ const create = context => {
const elementReference = arrayReferences.find(reference => {
const node = reference.identifier.parent;

if (node.parent.type !== 'VariableDeclarator') {
return false;
}

return true;
return node.parent.type === 'VariableDeclarator';
});
const elementNode = elementReference && elementReference.identifier.parent.parent;
const elementIdentifierName = elementNode && elementNode.id.name;
Expand Down
10 changes: 2 additions & 8 deletions rules/no-static-only-class.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -51,17 +51,11 @@ function isStaticMember(node) {
}

// TypeScript class
if (
isDeclare ||
return !(isDeclare ||
isReadonly ||
typeof accessibility !== 'undefined' ||
(Array.isArray(decorators) && decorators.length > 0) ||
key.type === 'TSPrivateIdentifier'
) {
return false;
}

return true;
key.type === 'TSPrivateIdentifier');
}

function * switchClassMemberToObjectProperty(node, sourceCode, fixer) {
Expand Down
Loading