feat: added basic eslint to repo #430
Conversation
.eslintrc.js
Outdated
| // Avoid accidental use of "==" instead of "===". | ||
| eqeqeq: 'error', | ||
| '@typescript-eslint/no-namespace': 'error', | ||
| '@typescript-eslint/no-floating-promises': 'off', // Temporarily disabled |
There was a problem hiding this comment.
I would prefer this enabled - it allows to catch when people forget to await.
There was a problem hiding this comment.
Definitely valuable to enable this, but if the codebase isn't prepared for it, it may trigger a lot of warnings. Worth a try though. But probably don't want to block this PR if the work can be done in a followup.
There was a problem hiding this comment.
should be pretty simple to enable it in runtimes
.eslintrc.js
Outdated
| '@typescript-eslint/no-floating-promises': 'off', // Temporarily disabled | ||
| '@typescript-eslint/no-redundant-type-constituents': 'off', // Temporarily disabled | ||
| '@typescript-eslint/no-unused-vars': 'error', | ||
| '@typescript-eslint/no-explicit-any': 'off', // Temporarily disabled |
There was a problem hiding this comment.
Being able to opt-out using explict any is valuable. It's explicit, so why not?
.eslintrc.js
Outdated
| 'unicorn/no-array-for-each': 'error', | ||
| 'security-node/detect-child-process': 'error', | ||
|
|
||
| // TODO: do we want to enable this? |
There was a problem hiding this comment.
Can we remove these comments please?
There was a problem hiding this comment.
Why? These need attention. Without the TODO, future devs must assume that these were intentional decisions.
There was a problem hiding this comment.
I think TODO is for something that has concrete action plan. If we just add it as a comment, it will stay here forever most likely. I personally do not think adding this header is anything high priority for now.
.eslintrc.js
Outdated
| mocha: true, | ||
| es2024: true, | ||
| }, | ||
| plugins: ['@typescript-eslint', '@stylistic', 'unicorn', 'header', 'security-node'], |
There was a problem hiding this comment.
Does this codebase already have unicorn etc. in its packages.json dependencies list? If not, they would need to be added.
There was a problem hiding this comment.
Included a more minimal amount of rules without any unicorn rules for now so I only kept the ts-eslint plugin
.eslintrc.js
Outdated
| selector: ['objectLiteralProperty', 'classMethod', 'enumMember'], | ||
| format: null, | ||
| }, | ||
| // Allow constants to use UPPER_CASE |
There was a problem hiding this comment.
UPPER_CASE is really over-used and counterproductive (in this codebase), but too late to change it in this codebase.
There was a problem hiding this comment.
Removed the rule for now, don't think it really adds any value
anasstahr
force-pushed
the
anasstahr/add-linting
branch
from
5fb3bf4 to
887ce89
Compare
|
Removed a lot of the rules, we can keep on adding but kept a minimal set. Also kept some of them off, since i'm personally not that familiar with some of these errors. |
runtimes/runtimes/chat/baseChat.ts
Outdated
| @@ -124,15 +124,21 @@ export class BaseChat implements Chat { | |||
| } | |||
|
|
|||
| public sendChatUpdate(params: ChatUpdateParams) { | |||
| this.connection.sendNotification(chatUpdateNotificationType.method, params) | |||
| this.connection | |||
There was a problem hiding this comment.
It would be better to change these methods return Promise<void>.
| this.lspConnection.client | ||
| .register(DidChangeConfigurationNotification.type, undefined) | ||
| .then(() => {}) | ||
| .catch(() => {}) |
There was a problem hiding this comment.
Could we log an error here?
| @@ -31,7 +31,7 @@ export class LspServer { | |||
| readonly notification: Notification | |||
|
|
|||
| private didChangeConfigurationHandler?: NotificationHandler<DidChangeConfigurationParams> | |||
| private executeCommandHandler?: RequestHandler<ExecuteCommandParams, any | undefined | null, void> | |||
| private executeCommandHandler?: RequestHandler<ExecuteCommandParams, unknown, void> | |||
There was a problem hiding this comment.
Why did we need this change?
There was a problem hiding this comment.
any already captures undefined and null so there were redundant types. I assume there can be any value in the handler. From what I saw online unknown is more preferred over any in that case
types/chat.ts
Outdated
| @@ -145,7 +145,7 @@ export interface QuickActionCommand { | |||
| } | |||
|
|
|||
| export type ContextCommandIconType = 'file' | 'folder' | 'code-block' | 'list-add' | 'magic' | |||
| export type IconType = ContextCommandIconType | 'help' | 'trash' | 'search' | 'calendar' | string | |||
| export type IconType = 'ContextCommandIconType' | 'help' | 'trash' | 'search' | 'calendar' | |||
There was a problem hiding this comment.
This is a breaking change - I do not think we can make it. Also, 'ContextCommandIconType' is different from the type above adding more icon types to this type?
There was a problem hiding this comment.
This is a breaking change
Yeah I wasn't so sure how to change this typing. Do we just want to keep it as astringtype and remove the other hardcoded strings or do we just disable the eslint rule for this specific line
'ContextCommandIconType' ...
Missed that one, thanks for pointing it out
anasstahr
force-pushed
the
anasstahr/add-linting
branch
from
2adf4aa to
ac8c19e
Compare
anasstahr
force-pushed
the
anasstahr/add-linting
branch
from
66a41e6 to
7689fe2
Compare
Problem
There currently doesn't exist any linting in this repo
Solution
Add linting by using VScode's team eslint as a template. Copied over minimal amount from their .eslint without custom rules.
I'm also adding service changes to fix the linting errors later, however currently this is a draft of just the eslint changes.
License
By submitting this pull request, I confirm that my contribution is made under the terms of the Apache 2.0 license.