-
-
Notifications
You must be signed in to change notification settings - Fork 368
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
prefer-string-starts-ends-with
: add suggestions for safely handling non-strings
#1277
prefer-string-starts-ends-with
: add suggestions for safely handling non-strings
#1277
Conversation
991a7c3
to
458dfdb
Compare
Use generator function instead of return array, should make function more clear |
prefer-string-starts-ends-with
: add suggestions for handling non-existent stringsprefer-string-starts-ends-with
: add suggestions for safely handling non-strings
* main: Temporarily disable `import/extensions` rule Update `xo` to v0.40 (sindresorhus#1257) Test: Better `output` assertion (sindresorhus#1259) `prefer-negative-index`: Refactor (sindresorhus#1255) `prefer-dom-node-remove`: Improve parentheses handling (sindresorhus#1254) Update `eslint-plugin-eslint-plugin` to v3.0.3 (sindresorhus#1256) 32.0.1 `prefer-set-has`: Remove outdated AST types (sindresorhus#1253) `prevent-abbreviations`: Fix shorthand import/export detection (sindresorhus#1252) 32.0.0 `no-for-loop`: Ignore known non-array loop variables (sindresorhus#1242) Add `no-document-cookie` rule (sindresorhus#1244) `prefer-array-find`: Singularize variable name in autofix (sindresorhus#1243) Update `@babel/core` (sindresorhus#1241) Mention libraries used under the hood in `better-regexp` rule doc (sindresorhus#1240) `prefer-switch`: Do not add braces to the default case (sindresorhus#1235)
The code fix part still need more work , if you don't mind, I can fix it later, unless you prefer do it yourself, I can guide. Another thing, I think we should remove auto fix for known non-strings, yes auto-fix will throw on runtime, but not every line is test covered, or even not reachable. You need fix this. |
@fisker feel free to push tweaks if you want. I will keep working on it too, appreciate your guidance! |
I pushed some changes, this corrects the fix logic, this add missing parens when the target is ternary |
rules/utils/should-add-parentheses-to-logical-expression-child.js
Outdated
Show resolved
Hide resolved
@bmish Can you fix conflicts? |
Removed suggestions for known string. Updated message, I'm not good at English, but I think it's more useful. |
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.
The code looks good, few other comments.
Thanks @bmish
Co-authored-by: fisker Cheung <lionkay@gmail.com>
// Goal: `String(target).startsWith(pattern)` | ||
case FIX_TYPE_STRING_CASTING: | ||
// `target` was a call argument, don't need check parentheses | ||
targetText = `String(${targetText})`; |
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 adds extra parenthesis that aren't needed sometimes. I think I had code to handle this before.
in: (/^b/).test((a))
out: ((a)).startsWith('b')
Not a big deal though.
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 know that, but better "add missing parens" instead of "remove extra parens".
@fisker looks good, thanks for helping to finish it up. |
Fixes #1267.