Skip to content

Add ESLint recommended rules #776

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

Merged
merged 14 commits into from
Jul 16, 2022
Merged

Add ESLint recommended rules #776

merged 14 commits into from
Jul 16, 2022

Conversation

auscompgeek
Copy link
Member

@auscompgeek auscompgeek commented Jun 19, 2022

Part of #714

Adds the following ESLint rules:

Ran yarn lint --fix then fixed the unautofixable lints.

Todo:

src/extension.ts Outdated
@@ -25,7 +25,6 @@ export async function activate(context: vscode.ExtensionContext) {
commandServerApi: () => commandServerApi,
getNodeAtLocation: () => getNodeAtLocation,
} as FactoryMap<Graph>);
graph.debug.init();
Copy link
Member Author

Choose a reason for hiding this comment

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

Debug#init was empty, so removed it altogether, since it's not implementing anything.

Copy link
Member

Choose a reason for hiding this comment

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

The way the graph works, accessing an attribute will cause the component to be instantiated, so it's possible this line was actually doing something

Copy link
Member

Choose a reason for hiding this comment

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

I believe this is the only remaining issue on this PR

Copy link
Member Author

Choose a reason for hiding this comment

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

Reverted this change. I'm not convinced this line is doing much in practice, but we can look into it later.

@auscompgeek
Copy link
Member Author

Might leave explicit-function-return-type for later, that one might get a bit gnarly.

@auscompgeek auscompgeek marked this pull request as ready for review June 19, 2022 07:13
@auscompgeek auscompgeek requested a review from pokey as a code owner June 19, 2022 07:13
Copy link
Member

@pokey pokey left a comment

Choose a reason for hiding this comment

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

Thank you! Left some initial comments

@@ -1,41 +1,39 @@
// @ts-nocheck
// From https://github.com/nodeca/js-yaml/issues/586#issuecomment-814310104
Copy link
Member

Choose a reason for hiding this comment

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

Epic! Presumably you tried running this? I don't believe it is actually tested. Best test is prob to run the "Update snapshots" debug launch config in VSCode

Copy link
Member Author

Choose a reason for hiding this comment

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

Huh, it seems like I may have accidentally made documentContents serialise as an inline string. I'll look into that.

Copy link
Member Author

Choose a reason for hiding this comment

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

Oh wait, that might just be because I have hard tabs by default, so the YAML serialiser wants to serialise a tab character as "\t":

documentContents: "try {\n\tif (true) {\n\t    const foo = \"hello\";\n\t}\n} catch (err) {\n\t\n}\n\ntry {\n\tconst bar = \"hello\";\n} catch (err) {\n\t\n}"

Copy link
Member

Choose a reason for hiding this comment

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

Yeah probably. Might be worth just changing your local setting temporarily so we don't have to wait on #731?

Copy link
Member Author

Choose a reason for hiding this comment

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

Okay, ran update snapshots with the default soft tabs, got this diff:

diff --git i/src/test/suite/fixtures/recorded/actions/snippets/snipDuplicatedDuplicatedHelloWorld.yml w/src/test/suite/fixtures/recorded/actions/snippets/snipDuplicatedDuplicatedHelloWorld.yml
index 4fb1c39ee51b..82281ea1bd5a 100644
--- i/src/test/suite/fixtures/recorded/actions/snippets/snipDuplicatedDuplicatedHelloWorld.yml
+++ w/src/test/suite/fixtures/recorded/actions/snippets/snipDuplicatedDuplicatedHelloWorld.yml
@@ -19,11 +19,11 @@ initialState:
 finalState:
   documentContents: >-
     This variable: 'hello_world' is duplicated here: 'hello_world', but '' is
     unique!
   selections:
-    - anchor: {line: 0, character: 69}
-      active: {line: 0, character: 69}
+    - anchor: {line: 0, character: 0}
+      active: {line: 0, character: 0}
   thatMark:
     - anchor: {line: 0, character: 0}
       active: {line: 0, character: 81}
 fullTargets: [{type: primitive, mark: {type: cursor}, modifiers: [{type: toRawSelection}]}]
diff --git i/src/test/suite/fixtures/recorded/actions/snippets/snipSpaghetti.yml w/src/test/suite/fixtures/recorded/actions/snippets/snipSpaghetti.yml
index 3393f857b693..2b89cc984297 100644
--- i/src/test/suite/fixtures/recorded/actions/snippets/snipSpaghetti.yml
+++ w/src/test/suite/fixtures/recorded/actions/snippets/snipSpaghetti.yml
@@ -13,11 +13,11 @@ initialState:
       active: {line: 0, character: 0}
   marks: {}
 finalState:
   documentContents: My friend  likes to eat spaghetti!
   selections:
-    - anchor: {line: 0, character: 10}
-      active: {line: 0, character: 10}
+    - anchor: {line: 0, character: 0}
+      active: {line: 0, character: 0}
   thatMark:
     - anchor: {line: 0, character: 0}
       active: {line: 0, character: 34}
 fullTargets: [{type: primitive, mark: {type: cursor}, selectionType: token, position: contents, insideOutsideType: inside, modifier: {type: identity}}]
diff --git i/src/test/suite/fixtures/recorded/selectionTypes/changeShortPaintParen.yml w/src/test/suite/fixtures/recorded/selectionTypes/changeShortPaintParen.yml
index a29af962ed1e..4889696e754e 100644
--- i/src/test/suite/fixtures/recorded/selectionTypes/changeShortPaintParen.yml
+++ w/src/test/suite/fixtures/recorded/selectionTypes/changeShortPaintParen.yml
@@ -1,6 +1,5 @@
- 
 command:
   spokenForm: change short paint paren
   version: 2
   targets:
     - type: primitive

The last one makes sense. What happened with the first two changes?

Copy link
Member

Choose a reason for hiding this comment

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

Ok well it has nothing to do with what you've done in this PR, so I'd consider this a passing test from that perspective.

As for why it's doing that, idk that's weird. Are tests passing for you locally?

Copy link
Member Author

Choose a reason for hiding this comment

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

Oddly enough they are passing locally after blowing away those changes. 🤔

Copy link
Member

@pokey pokey Jul 4, 2022

Choose a reason for hiding this comment

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

Strange. Possible that test fails nondeterministically. We retry all tests 5 times because VSCode tests are generally flakey for us, but when you do a snapshot update it will just use the first result so any failure will just become a snapshot update and not re-run

@auscompgeek auscompgeek requested a review from pokey July 16, 2022 15:20
Copy link
Member

@pokey pokey left a comment

Choose a reason for hiding this comment

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

Looks good thanks!

@pokey pokey merged commit ca1aa9d into main Jul 16, 2022
@pokey pokey deleted the auscompgeek/eslint branch July 16, 2022 15:36
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants