Skip to content
Merged
Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
35 commits
Select commit Hold shift + click to select a range
8f4c931
Initial pre-commit config
pokey Apr 15, 2022
c9a234b
Updates to pre commit
pokey Apr 20, 2022
4a400c2
New prettier config
pokey Apr 20, 2022
aef8148
Fix type
pokey Apr 20, 2022
cf46fb9
Add comment
pokey Apr 20, 2022
6f752db
At another comment to
pokey Apr 20, 2022
db2c48b
Tweak pretty or config
pokey Apr 20, 2022
70627f4
More pre commit fixes
pokey Apr 20, 2022
d0ccaf7
Improve comment
pokey Apr 20, 2022
73d776d
Improve comment
pokey Apr 20, 2022
843e72f
Add prettier recommended extension
pokey Apr 20, 2022
f471437
Merge branch 'main' into pokey-pre-commit
pokey Apr 20, 2022
a129e30
Fix typescript errors
pokey Apr 20, 2022
3d2a717
Add black to vscode settings
pokey Apr 21, 2022
dcc3f5c
Change prettier version
pokey Apr 21, 2022
c3f6c27
Don't autofix PRs
pokey Apr 21, 2022
1632849
Add simple docs
pokey Apr 21, 2022
37272b2
Tweak editor config
pokey Apr 21, 2022
d744b35
At a comment
pokey Apr 21, 2022
ed295a0
Removes something we didn't need
pokey Apr 21, 2022
ec530db
Remove marked down line length
pokey Apr 21, 2022
6f5c36a
Attempt to fix eslint in pre comet
pokey Apr 21, 2022
154d08b
Fix
pokey Apr 21, 2022
d958e09
Another fix
pokey Apr 21, 2022
32a0e64
Try to fix eslint
pokey Apr 21, 2022
03bd12d
Bump versions
pokey Apr 21, 2022
0e525eb
Update lock file
pokey Apr 21, 2022
1308ce7
Tweak package
pokey Apr 21, 2022
316b873
unset editorconfig for vendor
pokey Apr 21, 2022
cb30dcb
Exclude vendor
pokey Apr 21, 2022
696ce82
Tweak editorconfig
pokey Apr 21, 2022
b726555
Add more rules
pokey Apr 22, 2022
aaf428a
Remove lint from pre commit
pokey Apr 22, 2022
61ffb5d
Sort hooks
pokey Apr 22, 2022
8b4abd3
Run shed last
pokey Apr 24, 2022
File filter

Filter by extension

Filter by extension


Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
38 changes: 38 additions & 0 deletions .editorconfig
Original file line number Diff line number Diff line change
@@ -0,0 +1,38 @@
# See https://EditorConfig.org
root = true

[*]
charset = utf-8
end_of_line = lf
insert_final_newline = true
indent_size = 2
indent_style = space
max_line_length = 80
trim_trailing_whitespace = true

[*.json]
indent_style = tab

[*.py]
indent_style = space
indent_size = 4

[*.{yml,yaml}]
# Trailing whitespace breaks yaml files if you use a multiline string
# with a line that has trailing white space. Many of our recorded
# tests use strings with trailing white space to represent the final
# document contents. For example
# src/test/suite/fixtures/recorded/languages/ruby/changeCondition.yml
trim_trailing_whitespace = false

[Makefile]
indent_style = tab

[src/vendor/**]
charset = unset
end_of_line = unset
indent_size = unset
indent_style = unset
trim_trailing_whitespace = unset
insert_final_newline = unset
max_line_length = unset
4 changes: 3 additions & 1 deletion .eslintrc.json
Original file line number Diff line number Diff line change
@@ -1,6 +1,9 @@
{
"root": true,
"parser": "@typescript-eslint/parser",
"extends": [
"prettier"
],
Comment on lines +4 to +6
Copy link
Member Author

Choose a reason for hiding this comment

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

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

"parserOptions": {
"ecmaVersion": 6,
"sourceType": "module"
Expand All @@ -10,7 +13,6 @@
],
"rules": {
"@typescript-eslint/naming-convention": "warn",
"@typescript-eslint/semi": "warn",
"@typescript-eslint/no-unused-vars": [
"warn",
{
Expand Down
41 changes: 41 additions & 0 deletions .pre-commit-config.yaml
Original file line number Diff line number Diff line change
@@ -0,0 +1,41 @@
minimum_pre_commit_version: "2.9.0"
ci:
autofix_prs: false
exclude: ^src/vendor/
repos:
- repo: https://github.com/pre-commit/pre-commit-hooks
Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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

Copy link
Member Author

@pokey pokey Apr 25, 2022

Choose a reason for hiding this comment

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

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

rev: v4.1.0
hooks:
- id: check-added-large-files
- id: check-case-conflict
- id: check-executables-have-shebangs
- id: check-merge-conflict
- id: check-shebang-scripts-are-executable
- id: check-symlinks
- id: destroyed-symlinks
- id: detect-private-key
- id: end-of-file-fixer
- id: fix-byte-order-marker
- id: mixed-line-ending
- id: trailing-whitespace
# Trailing whitespace breaks yaml files if you use a multiline string
# with a line that has trailing white space. Many of our recorded
# tests use strings with trailing white space to represent the final
# document contents. For example
# src/test/suite/fixtures/recorded/languages/ruby/changeCondition.yml
exclude: ^src/test/suite/fixtures/recorded/.*/[^/]*\.yml$
- repo: https://github.com/pre-commit/mirrors-prettier
rev: "v2.6.2"
hooks:
- id: prettier
- repo: https://github.com/ikamensh/flynt/
rev: "0.76"
hooks:
- id: flynt
- repo: https://github.com/Zac-HD/shed
rev: 0.9.5
hooks:
- id: shed
# TODO: bump to --py310-plus when Talon moves to Python 3.10.
args: [--refactor, --py39-plus]
types_or: [python, markdown, rst]
4 changes: 4 additions & 0 deletions .prettierignore
Original file line number Diff line number Diff line change
@@ -0,0 +1,4 @@
/src/vendor

# We use our own format for our recorded yaml tests to keep them compact
/src/test/suite/fixtures/recorded/**/*.yml
3 changes: 2 additions & 1 deletion .vscode/extensions.json
Original file line number Diff line number Diff line change
Expand Up @@ -2,6 +2,7 @@
// See http://go.microsoft.com/fwlink/?LinkId=827846
// for the documentation about the extensions.json format
"recommendations": [
"dbaeumer.vscode-eslint"
"dbaeumer.vscode-eslint",
"esbenp.prettier-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.

This will probably be nice for new contributors

]
}
7 changes: 3 additions & 4 deletions .vscode/settings.json
Original file line number Diff line number Diff line change
Expand Up @@ -6,9 +6,8 @@
"search.exclude": {
"out": true // set this to false to include "out" folder in search results
},
"[typescript]": {
"editor.defaultFormatter": "esbenp.prettier-vscode"
},
"python.formatting.provider": "black",
"editor.defaultFormatter": "esbenp.prettier-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.

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

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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

// Turn off tsc task auto detection since we have the necessary tasks as npm scripts
"typescript.tsc.autoDetect": "off",
"cSpell.words": [
Expand All @@ -21,4 +20,4 @@
"pojo",
"subword"
]
}
}
7 changes: 7 additions & 0 deletions docs/contributing/CONTRIBUTING.md
Original file line number Diff line number Diff line change
Expand Up @@ -18,6 +18,13 @@ locally you need to run the extension in debug mode. To do so you need to run
the `workbench.action.debug.selectandstart` command and then select either "Run
Extension" or "Extension Tests".

### Code formatting

We use [`pre-commit`](https://pre-commit.com/) to automate autoformatting.
Autoformatters will automatically run on PRs in CI, but you can also run them
locally or install pre-commit hooks as described in the `pre-commit`
documentation.

### Running docs site locally

Run the `workbench.action.debug.selectandstart` command and then select
Expand Down
10 changes: 6 additions & 4 deletions package.json
Original file line number Diff line number Diff line change
Expand Up @@ -513,15 +513,17 @@
"@types/semver": "^7.3.9",
"@types/sinon": "^10.0.2",
"@types/vscode": "^1.61.0",
"@typescript-eslint/eslint-plugin": "^5.11.0",
"@typescript-eslint/parser": "^5.11.0",
"@typescript-eslint/eslint-plugin": "^5.20.0",
"@typescript-eslint/parser": "^5.20.0",
"esbuild": "^0.11.12",
"eslint": "^7.15.0",
"eslint": "^8.13.0",
"eslint-config-prettier": "^8.5.0",
"fast-xml-parser": "^3.20.0",
"glob": "^7.1.7",
"js-yaml": "^4.1.0",
"mocha": "^8.1.3",
"npm-license-crawler": "^0.2.1",
"prettier": "2.6.2",
Copy link
Collaborator

Choose a reason for hiding this comment

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

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

Copy link
Member

Choose a reason for hiding this comment

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

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.

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Member Author

Choose a reason for hiding this comment

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

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

Copy link
Collaborator

Choose a reason for hiding this comment

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

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.

Copy link
Collaborator

Choose a reason for hiding this comment

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

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)

Copy link
Member Author

@pokey pokey Apr 22, 2022

Choose a reason for hiding this comment

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

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?

"semver": "^7.3.5",
"sinon": "^11.1.1",
"typescript": "^4.5.5",
Expand All @@ -532,4 +534,4 @@
"immutability-helper": "^3.1.1",
"lodash": "^4.17.21"
}
}
}
6 changes: 3 additions & 3 deletions src/core/updateSelections/updateSelections.ts
Original file line number Diff line number Diff line change
Expand Up @@ -66,7 +66,7 @@ export function getSelectionInfo(
*/
export function selectionsToSelectionInfos(
document: TextDocument,
selectionMatrix: Selection[][],
selectionMatrix: (readonly Selection[])[],
Copy link
Member Author

Choose a reason for hiding this comment

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

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

rangeBehavior: DecorationRangeBehavior = DecorationRangeBehavior.ClosedClosed
): FullSelectionInfo[][] {
return selectionMatrix.map((selections) =>
Expand Down Expand Up @@ -118,7 +118,7 @@ export async function callFunctionAndUpdateSelections(
rangeUpdater: RangeUpdater,
func: () => Thenable<unknown>,
document: TextDocument,
selectionMatrix: Selection[][]
selectionMatrix: (readonly Selection[])[]
): Promise<Selection[][]> {
const selectionInfoMatrix = selectionsToSelectionInfos(
document,
Expand Down Expand Up @@ -173,7 +173,7 @@ export async function performEditsAndUpdateSelections(
rangeUpdater: RangeUpdater,
editor: TextEditor,
edits: Edit[],
originalSelections: Selection[][]
originalSelections: (readonly Selection[])[]
) {
const document = editor.document;
const selectionInfoMatrix = selectionsToSelectionInfos(
Expand Down
12 changes: 7 additions & 5 deletions src/util/addDecorationsToEditor.ts
Original file line number Diff line number Diff line change
Expand Up @@ -20,15 +20,17 @@ export function addDecorationsToEditors(
) {
hatTokenMap.clear();

var editors: vscode.TextEditor[];
var editors: readonly vscode.TextEditor[];

if (vscode.window.activeTextEditor == null) {
editors = vscode.window.visibleTextEditors;
} else {
editors = vscode.window.visibleTextEditors.filter(
(editor) => editor !== vscode.window.activeTextEditor
);
editors.unshift(vscode.window.activeTextEditor);
editors = [
vscode.window.activeTextEditor,
...vscode.window.visibleTextEditors.filter(
(editor) => editor !== vscode.window.activeTextEditor
),
];
Comment on lines -31 to +33
Copy link
Member Author

Choose a reason for hiding this comment

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

We can't use unshift on a readonly array

}

const tokens = concat(
Expand Down
8 changes: 4 additions & 4 deletions third-party-licenses.csv
Original file line number Diff line number Diff line change
@@ -1,12 +1,12 @@
"module name","licenses","repository","licenseUrl","parents"
"@docusaurus/core@2.0.0-beta.15","MIT","https://github.com/facebook/docusaurus","https://github.com/facebook/docusaurus/raw/master/LICENSE","website"
"@docusaurus/preset-classic@2.0.0-beta.15","MIT","https://github.com/facebook/docusaurus","https://github.com/facebook/docusaurus/raw/master/LICENSE","website"
"@docusaurus/core@2.0.0-beta.17","MIT","https://github.com/facebook/docusaurus","https://github.com/facebook/docusaurus/raw/master/LICENSE","website"
"@docusaurus/preset-classic@2.0.0-beta.17","MIT","https://github.com/facebook/docusaurus","https://github.com/facebook/docusaurus/raw/master/LICENSE","website"
"@mdx-js/react@1.6.22","MIT","https://github.com/mdx-js/mdx","https://github.com/mdx-js/mdx/raw/master/license","website"
"@types/lodash@4.14.168","MIT","https://github.com/DefinitelyTyped/DefinitelyTyped","https://github.com/DefinitelyTyped/DefinitelyTyped/raw/master/LICENSE","cursorless"
"@types/lodash@4.14.181","MIT","https://github.com/DefinitelyTyped/DefinitelyTyped","https://github.com/DefinitelyTyped/DefinitelyTyped/raw/master/LICENSE","cursorless"
"clsx@1.1.1","MIT","https://github.com/lukeed/clsx","https://github.com/lukeed/clsx/raw/master/license","website"
"immutability-helper@3.1.1","MIT","https://github.com/kolodny/immutability-helper","https://github.com/kolodny/immutability-helper/raw/master/LICENSE","cursorless"
"mdast-util-find-and-replace@2.1.0","MIT","https://github.com/syntax-tree/mdast-util-find-and-replace","https://github.com/syntax-tree/mdast-util-find-and-replace/raw/master/license","website"
"prism-react-renderer@1.2.1","MIT","https://github.com/FormidableLabs/prism-react-renderer","https://github.com/FormidableLabs/prism-react-renderer/raw/master/LICENSE","website"
"prism-react-renderer@1.3.1","MIT","https://github.com/FormidableLabs/prism-react-renderer","https://github.com/FormidableLabs/prism-react-renderer/raw/master/LICENSE","website"
"react-dom@17.0.2","MIT","https://github.com/facebook/react","https://github.com/facebook/react/raw/master/LICENSE","website"
"react@17.0.2","MIT","https://github.com/facebook/react","https://github.com/facebook/react/raw/master/LICENSE","website"
"unist-util-visit@4.1.0","MIT","https://github.com/syntax-tree/unist-util-visit","https://github.com/syntax-tree/unist-util-visit/raw/master/license","website"
Loading