fix lint warnings caused by recent ESLint configuration update #5031

jgravois · 2020-06-17T03:24:13Z

resolves #5026

fix trailing comma and indentation warnings
turn off typescript-eslint indent rule (to workaround myriad issues)
add manual single line ignore (i think that is the right move 🤷🏼‍♂️ anyway)

disclaimer: i know that code style is a perennially prickly topic and i am a complete outsider to this codebase so i assure you no offense will be taken if any of my choices here are called into question.

Checklist

It's really useful if your PR relates to an outstanding issue, so please reference it in your PR, or create an explanatory one for discussion. In many cases features are absent for a reason.
This message body should clearly illustrate what problems it solves. If there are related issues, remember to reference them.
Ideally, include a test that fails without this PR but passes with it. PRs will only be merged once they pass CI. (Remember to npm run lint!)

Tests

Run the tests tests with npm test or yarn test)

* fix trailing comma and indentation warnings * turn off typescript-eslint indent rule (to workaround myriad issues) * add manual single line ignore (i _think_ that is the right move 🤷🏼‍♂️ anyway)

benmccann · 2020-06-17T03:30:58Z

.eslintrc.js

+	},
+	// workaround for https://github.com/typescript-eslint/typescript-eslint/issues/1824 and rely solely on 'indent' rule
+	rules: {
+		"@typescript-eslint/indent": "off"


We should probably change this in the common config:

https://github.com/sveltejs/eslint-config/blob/e8a9b27cd3f7aa66388474412b1a5c11c5a44ade/index.js#L31

sounds good. i've opened sveltejs/eslint-config#1 and removed the custom rule i proposed initially here in 52eee5e.

jgravois · 2020-06-17T03:27:03Z

test/helpers.js

@@ -153,7 +153,7 @@ export function normalizeHtml(window, html) {
 export function setupHtmlEqual() {
 	const window = env();

-	assert.htmlEqual = (actual, expected, message) => {
+	assert.htmlEqual = (actual, expected, message) => { // eslint-disable-line no-import-assign


this is the single line ignore i mentioned above. it suppresses the following warning:

../svelte/test/helpers.js 156:2 warning The members of 'assert' are read-only no-import-assign

do we need it?

i'm not sure what you mean. .htmlEqual is certainly in use in the test suite.

https://github.com/sveltejs/svelte/search?q=htmlEqual&unscoped_q=htmlEqual

jgravois · 2020-06-17T03:33:35Z

.eslintrc.js

+	},
+	// workaround for https://github.com/typescript-eslint/typescript-eslint/issues/1824 and rely solely on 'indent' rule
+	rules: {
+		"@typescript-eslint/indent": "off"
 	}


prior to making this change, it was not possible to use --fix to resolve the lint warnings reported by 'indent' itself.

this leads me to believe that 'indent' and '@typescript-eslint/indent" are at an impasse as to what the appropriate indentation should be. given the language in typescript-eslint/typescript-eslint#1824, it seemed safest to side with 'indent'.

relies on sveltejs/eslint-config#1

benmccann · 2020-08-12T22:58:22Z

Thanks for taking the initiative on this. I'm going to close this one in favor of #5263 which doesn't make whitespace changes

lint for the lint gods

354ab15

* fix trailing comma and indentation warnings * turn off typescript-eslint indent rule (to workaround myriad issues) * add manual single line ignore (i _think_ that is the right move 🤷🏼‍♂️ anyway)

benmccann reviewed Jun 17, 2020

View reviewed changes

jgravois commented Jun 17, 2020

View reviewed changes

jgravois mentioned this pull request Jun 17, 2020

turn off @typescript-eslint/indent and rely exclusively on indent sveltejs/eslint-config#1

Closed

instead of a custom rule, make the change upstream

52eee5e

relies on sveltejs/eslint-config#1

benmccann closed this Aug 12, 2020

jgravois deleted the patch-5026 branch August 17, 2020 16:58

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

fix lint warnings caused by recent ESLint configuration update #5031

fix lint warnings caused by recent ESLint configuration update #5031

jgravois commented Jun 17, 2020

benmccann Jun 17, 2020

jgravois Jun 17, 2020

jgravois Jun 17, 2020

PhilipJohnBasile Jul 18, 2020

jgravois Jul 20, 2020

jgravois Jun 17, 2020

benmccann commented Aug 12, 2020

fix lint warnings caused by recent ESLint configuration update #5031

fix lint warnings caused by recent ESLint configuration update #5031

Conversation

jgravois commented Jun 17, 2020

Checklist

Tests

benmccann Jun 17, 2020

Choose a reason for hiding this comment

jgravois Jun 17, 2020

Choose a reason for hiding this comment

jgravois Jun 17, 2020

Choose a reason for hiding this comment

PhilipJohnBasile Jul 18, 2020

Choose a reason for hiding this comment

jgravois Jul 20, 2020

Choose a reason for hiding this comment

jgravois Jun 17, 2020

Choose a reason for hiding this comment

benmccann commented Aug 12, 2020