Add eslint to project

[kpodsiad]

todo

• use prettier for formatting rules (e.g. quotes)
• setup CI check
• add extensions.json with recommended extensions

[gabro]

Thanks @kpodsiad! I have many comments to this, but I won't be able to look at it for a few days, unless we're in a rush can we please hold on to it for a bit? Two general comments I can leave now:

• let's not use eslint for formatting rules (quotes, braces, etc). Let's stick to prettier for it, which is more appropriate, with whatever default it has
• as suggested on discord, I would start smaller, I already see many rules with questionable benefits that require us to turn them off selectively. It's a small codebase with relatively few contributors, I think we can afford going little by little

[gabro]

Another couple of suggestions for improving dx:

• setting up recommended extensions for the workspace, so that new contributors opening the project will get a prompt suggesting to install them
• setting up lint-staged to run prettier and eslint on changed files as a pre commit hook
• enforce eslint on the CI

[kpodsiad]

Thanks for your feedback @gabro
I'll add todo list containing your suggestions, but one of them needs further discussion. We aren't in rush so don't worry and take your time ;)

• let's not use eslint for formatting rules (quotes, braces, etc). Let's stick to prettier for it, which is more appropriate, with whatever default it has

Yes, you are right with quotes, it could be managed by prettier. However, braces should be managed by eslint, see this comment.

• as suggested on discord, I would start smaller, I already see many rules with questionable benefits that require us to turn them off selectively. It's a small codebase with relatively few contributors, I think we can afford going little by little

I started with just recommended rules, I disabled only 3 rules so it's not that many. I think it's better to stick with the recommended ones and disable the rules which don't suit us than another way, it's shorter for sure.

• setting up lint-staged to run prettier and eslint on changed files as a pre commit hook

Pre-commit hook could very annoying, especially when developing sth locally and frequently committing local changes. For now, I wouldn't add it.

[gabro]

@kpodsiad coming back to this

Yes, you are right with quotes, it could be managed by prettier. However, braces should be managed by eslint, see this comment.

Ok, I'm still not super sold on enforcing formatting rules with eslint because it's trickier to setup a "format on save" workflow. In order to fix those errors on save you would need to enable code actions on save in VS Code, but that setting also fixes a bunch of other non-formatting stuff, so it's not something we would necessarily want to recommend.

I started with just recommended rules, I disabled only 3 rules so it's not that many. I think it's better to stick with the recommended ones and disable the rules which don't suit us than another way, it's shorter for sure.

Left some comment inline about this

Pre-commit hook could very annoying, especially when developing sth locally and frequently committing local changes. For now, I wouldn't add it.

Sure, let's avoid it for now. I would still enforce in the CI, otherwise we're still at risk of merging something that doesn't comply with the rules, which in turn generates spurious diffs in future PRs.

[kpodsiad]

Ok, I'm still not super sold on enforcing formatting rules with eslint because it's trickier to setup a "format on save" workflow. In order to fix those errors on save you would need to enable code actions on save in VS Code, but that setting also fixes a bunch of other non-formatting stuff, so it's not something we would necessarily want to recommend.

@gabro Do you think that lack of "fix on save" action is problematic? ESLint extension shows errors immediately and offers the possibility to quick-fix them. I think, in the terms of this particular rule which is curly, what ESLint does is good enough. Especially when we'll take into account amount of people who will be developing metals extension and fact that inserting braces is a little bit more than just formatting ;)

[gabro]

Thanks for addressing my comments! I think it's good enough to merge, let's try with curly and see how it goes :)

There's a merge conflict to fix, but other than that, LGTM! Nice work!

[kpodsiad]

@tgodzik @gabro could we merge this PR instead of labeling this with wait for metals release?

[tgodzik]

@tgodzik @gabro could we merge this PR instead of labeling this with wait for metals release?

We don't need to wait for anything that doesn't require server changes. I am ok with merging, though I am not an expert here 😅

[gabro]

Nothing here that needs a new metals release, so yes, let's merge it!

[kpodsiad]

In theory, syntax/formatting changes could break some features, just saying' :P