-
-
Notifications
You must be signed in to change notification settings - Fork 0
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
Run sindresorhus#1221 on codebase #282
base: prefer-ternary-improve
Are you sure you want to change the base?
Conversation
babel-eslint
(#1211)if (callback.body.type !== 'BlockStatement') { | ||
return false; | ||
} | ||
|
||
return true; | ||
return callback.body.type === 'BlockStatement'; |
There was a problem hiding this comment.
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 😄
} | ||
|
||
return isSameScope(parent, resolved.scope); | ||
return definition && definition.type === 'FunctionName' && resolved.name === definition.name.name ? false : isSameScope(parent, resolved.scope); |
There was a problem hiding this comment.
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:
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); |
There was a problem hiding this comment.
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:
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):
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
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I tried reviewing many and it looks like:
- if it's a clear
return true + return false
, definite improvement - if it's
return false
+return other condition
, thefalse
might get lost in the ternary - if the condition is long enough, the ternary should be multi-line
- if the resulting ternary is longer than 30/50 characters it starts to lose readability
Maybe as a general rule, prefer-ternary
is an improvement if the 2 options are either short or visually similar. ? a : b
is ok, but long(testing).here() ? a : some == long.condition('maybe')
is less ok.
} | ||
|
||
return afterArguments; | ||
return dropColon ? afterArguments.slice(1).trim() : afterArguments; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is good
} | ||
|
||
return {}; | ||
isCompareLeft(node, '<=', 1) ? {isZeroLengthCheck: false, node} : {}; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is fine
} | ||
|
||
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]}`; |
There was a problem hiding this comment.
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
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]}`; |
There was a problem hiding this comment.
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]}`;
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Indeed
There was a problem hiding this comment.
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
} | ||
|
||
return new RegExp(item, 'u'); | ||
return item instanceof RegExp ? item : new RegExp(item, 'u'); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This is fine
} | ||
|
||
return { | ||
return filenameWithExtension === '<input>' || filenameWithExtension === '<text>' ? {} : { |
There was a problem hiding this comment.
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
} | ||
|
||
return greater.object.name; | ||
return greater.property.name !== 'length' ? undefined : greater.object.name; |
There was a problem hiding this comment.
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.
No description provided.