Add pre-commit #637

pokey · 2022-04-20T17:39:52Z

Adding pre-commit. To see what it looks like when we run it, see #638

See #639 and #640 for a couple minor follow-up tasks

pokey · 2022-04-20T17:41:16Z

.editorconfig

+
+# Tab indentation (no size specified)
+[*.json]
+indent_style = tab


This is what we're already using in most of our .json files, so I figured I'd formalize it here. Any objections? cc/ @Will-Sommers @AndreasArvidsson @phillco

pokey · 2022-04-20T17:46:53Z

.editorconfig

@@ -0,0 +1,27 @@
+# EditorConfig is awesome: https://EditorConfig.org


This file is the most generic way I know to set formatting preferences, so I argue we should use it as far as possible. It is respected by Prettier, so pre-commit will automatically apply these settings via Prettier

pokey · 2022-04-20T17:47:38Z

.eslintrc.json

+    "extends": [
+        "prettier"
+    ],


Let prettier handle all the formatting; eslint just checks for / fixes problems

pokey · 2022-04-20T18:20:22Z

.vscode/settings.json

-    "[typescript]": {
-        "editor.defaultFormatter": "esbenp.prettier-vscode"
-    },
+    "editor.defaultFormatter": "esbenp.prettier-vscode",


Note that we now use prettier for all file types that it supports

Have you confirmed that it's not trying to use prettier for python for example. "that it supports" it's not something I would rely on to work automatically.

Yeah I just tried saving and it seems to be using black

pokey · 2022-04-20T18:22:57Z

.vscode/extensions.json

 	"recommendations": [
-		"dbaeumer.vscode-eslint"
+		"dbaeumer.vscode-eslint",
+		"esbenp.prettier-vscode"


This will probably be nice for new contributors

pokey · 2022-04-20T19:03:48Z

src/core/updateSelections/updateSelections.ts

 export function selectionsToSelectionInfos(
  document: TextDocument,
-  selectionMatrix: Selection[][],
+  selectionMatrix: (readonly Selection[])[],


Looks like our typescript version got bumped when I bumped yarn.lock, so now we need to be stricter about readonly

pokey · 2022-04-20T19:04:12Z

src/util/addDecorationsToEditor.ts

-    editors.unshift(vscode.window.activeTextEditor);
+    editors = [
+      vscode.window.activeTextEditor,
+      ...vscode.window.visibleTextEditors.filter(
+        (editor) => editor !== vscode.window.activeTextEditor
+      ),
+    ];


We can't use unshift on a readonly array

Will-Sommers · 2022-04-20T20:17:46Z

package.json

 		"js-yaml": "^4.1.0",
 		"mocha": "^8.1.3",
 		"npm-license-crawler": "^0.2.1",
+		"prettier": "2.6.2",


Not a big thing but if you want to add the ability to shift up rather have a strict peg, that would be nice.

I think it makes sense to pin prettier in the package.json to the same version in the pre-commit config. As long as we remember to bump this whenever it gets bumped in the pre-commit config.

Hmm it's a bit of a bummer to have to specify prettier version in two places. I wonder if that's avoidable? 🤔

Ok I've now pinned everything in package.json that's specified in the pre-commit config, but that seems not great. I wonder if there's a better way

I'll open an issue on their repo and see what comes of it. I think that duplicating it is the path forward for now.

It looks like this is the resolution. The tldr; is to rely on a repository local hookand throw the check into an npm or yarn script.

pre-commit/pre-commit#945 (comment)

Ok, given that currently eslint is already run in CI, the eslint --fix isn't buying us a ton, so probably not worth the effort. Let's remove the eslint fixer for now, and wait until we start using it to remove unused imports in #640. I've added a note to that issue so we remember.

I think it's ok to leave prettier pinned until then as well. It was mainly the pinned eslint typescript parser plugin making me nervous

Sound good?

.pre-commit-config.yaml

Will-Sommers · 2022-04-20T20:36:39Z

Looks great to me.

.pre-commit-config.yaml

.editorconfig

auscompgeek · 2022-04-21T11:39:54Z

package.json

 		"js-yaml": "^4.1.0",
 		"mocha": "^8.1.3",
 		"npm-license-crawler": "^0.2.1",
+		"prettier": "2.6.2",


I think it makes sense to pin prettier in the package.json to the same version in the pre-commit config. As long as we remember to bump this whenever it gets bumped in the pre-commit config.

.pre-commit-config.yaml

pokey · 2022-04-21T17:12:53Z

Ok thanks for all the feedback! I think this one is good to go except for #637 (comment), so would be great if anyone has ideas there cc/ @Will-Sommers @auscompgeek

auscompgeek

LGTM. A couple of comments.

auscompgeek · 2022-04-23T06:13:55Z

.pre-commit-config.yaml

+        # TODO: bump to --py310-plus when Talon moves to Python 3.10.
+        args: [--refactor, --py39-plus]
+        types_or: [python, markdown, rst]
+  - repo: https://github.com/pre-commit/pre-commit-hooks


Might be a good idea to move pre-commit-hooks before shed? They should be faster I think; failing fast would probably be ideal.

Done. Fwiw, though, pre-commit doesn't short-circuit, so I'm not sure there's too much benefit

ah, I suppose you want to run black after flynt anyway

Yeah exactly. And as a matter of fact I did find it useful to be able to hit Ctrl-c when running locally if I noticed failure, so I guess there is some benefit 👍.

Thanks for all the helpful comments! I'm happy with how it turned out

.pre-commit-config.yaml

pokey added 11 commits April 15, 2022 07:16

Initial pre-commit config

8f4c931

Updates to pre commit

c9a234b

New prettier config

4a400c2

Fix type

aef8148

Add comment

cf46fb9

At another comment to

6f752db

Tweak pretty or config

db2c48b

More pre commit fixes

70627f4

Improve comment

d0ccaf7

Improve comment

73d776d

Add prettier recommended extension

843e72f

pokey commented Apr 20, 2022

View reviewed changes

pokey added 2 commits April 20, 2022 11:33

Merge branch 'main' into pokey-pre-commit

f471437

Fix typescript errors

a129e30

pokey commented Apr 20, 2022

View reviewed changes

pokey marked this pull request as ready for review April 20, 2022 19:04

pokey requested review from AndreasArvidsson and Will-Sommers April 20, 2022 19:04

Will-Sommers reviewed Apr 20, 2022

View reviewed changes

.pre-commit-config.yaml Show resolved Hide resolved

Will-Sommers reviewed Apr 20, 2022

View reviewed changes

.pre-commit-config.yaml Show resolved Hide resolved

Will-Sommers reviewed Apr 20, 2022

View reviewed changes

.pre-commit-config.yaml Outdated Show resolved Hide resolved

auscompgeek reviewed Apr 21, 2022

View reviewed changes

pokey added 5 commits April 21, 2022 13:57

Add black to vscode settings

3d2a717

Change prettier version

dcc3f5c

Don't autofix PRs

c3f6c27

Add simple docs

1632849

Tweak editor config

37272b2

pokey added 9 commits April 21, 2022 17:32

At a comment

d744b35

Removes something we didn't need

ed295a0

Remove marked down line length

ec530db

Attempt to fix eslint in pre comet

6f5c36a

Fix

154d08b

Another fix

d958e09

Try to fix eslint

32a0e64

Bump versions

03bd12d

Update lock file

0e525eb

pokey added 6 commits April 21, 2022 18:14

Tweak package

1308ce7

unset editorconfig for vendor

316b873

Exclude vendor

cb30dcb

Tweak editorconfig

696ce82

Add more rules

b726555

Remove lint from pre commit

aaf428a

pokey force-pushed the pokey-pre-commit branch from f70cfb4 to aaf428a Compare April 22, 2022 11:27

pokey mentioned this pull request Apr 22, 2022

Cleanup Typescript imports in pre-commit #640

Open

4 tasks

Sort hooks

61ffb5d

Will-Sommers previously approved these changes Apr 22, 2022

View reviewed changes

auscompgeek previously approved these changes Apr 23, 2022

View reviewed changes

Run shed last

8b4abd3

pokey dismissed stale reviews from auscompgeek and Will-Sommers via 8b4abd3 April 24, 2022 16:15

pokey merged commit 6256e45 into main Apr 24, 2022

pokey deleted the pokey-pre-commit branch April 24, 2022 16:26

		@@ -0,0 +1,27 @@
		# EditorConfig is awesome: https://EditorConfig.org

Uh oh!

Add pre-commit #637

Add pre-commit #637

Uh oh!

Conversation

pokey commented Apr 20, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pokey Apr 22, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Will-Sommers commented Apr 20, 2022

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

pokey commented Apr 21, 2022

Uh oh!

auscompgeek left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

pokey Apr 25, 2022 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

5 participants

pokey commented Apr 20, 2022 •

edited

Loading

pokey Apr 22, 2022 •

edited

Loading

pokey Apr 25, 2022 •

edited

Loading