[WIP] feat(diagnostics): ability to ignore some codes #172

pyrho · 2020-11-18T20:33:26Z

Two things remain:

Make it work 😅
Unit test pass... but
Update documentation here and on nvim-lspconfig

Fixed. ~~Regarding 1. It's working, but I'm failing at configuring neovim's LSP to spawn ts-lang-server with the new cli switch:~~
~~fails to spawn ts-lang-server entirely; removing my new options and it's back to normal.~~

Regarding 2., I can't make them fail :D the file snippet does not trigger the expected diagnostic for some reason I'm not clear why.

Sample config:

require'nvim_lsp'.tsserver.setup{
    cmd = { "typescript-language-server", "--stdio", "--ignored-diagnostic-codes", "80006" },
}

Will resolve #166 when merged.

rchl · 2020-11-18T20:57:15Z

server/src/diagnostic-queue.ts

+        const { file, diagnostics: diagnosticsFromServer } = event.body;
+
+        // `d.code` can be undefined
+        const isDiagnosticAllowed = (d: tsp.Diagnostic) => !d.code || this.ignoredDiagnosticCodes.indexOf(d.code) === -1;


[].includes() instead of indexOf maybe?

I used Array.every in my first version but that's why it wasn't working in neovim.

/lib/diagnostic-queue.js:50 const isDiagnosticAllowed = (d) => this.ignoredDiagnosticCodes.every(c => d.code !== c); TypeError: this.ignoredDiagnosticCodes.every is not a function

I'm not sure what's going on there, maybe an older nodeJS runtime; but I figured I'd do away with the headache and just stick to es5.

Array.includes is supported since node 6 though so it won't be a problem.

This project currently targets es6 (or 2015), and it looks like Array.includes is only available for es7 (2016) compilation targets.

I see. Then never mind me although that target should probably be updated in a separate PR. That target is very old.

I guess targeting an older standard has the added benefit of maximizing compatibility, if for some reason you're stuck with an older node version.

But ES7 is supported from Node7 (or Node 8 if you prefer "stable" release). It would be reasonable to require at least Node 8 as that is already 3 or 4 years old.

Agreed that targeting the current LTS version would be better.

I'd lean toward getting the target bumped prior to this PR if either of you are interested. Based on https://nodejs.org/en/about/releases/ we should probably support Node v10 and up

Edit: Created #173 to keep track of it

I agree bumping deps and updating the target JS version is high priority, but it's unrelated to the changes in this PR and shouldn't be a blocker for merging this imo.

server/src/lsp-connection.ts

pyrho · 2020-11-18T23:09:55Z

server/src/lsp-server.spec.ts

+        const diagnosticsForThisFile = diagnostics.filter(d => d!.uri === doc.uri);
+        assert.equal(diagnosticsForThisFile.length, 1, JSON.stringify(diagnostics));
+        const fileDiagnostics = diagnosticsForThisFile[0]!.diagnostics;
+        assert.equal(fileDiagnostics.length, 0);


This test passes but only because the sample TS code does not generate a diagnostic.

But in reality, a diagnostic is emitted

Hrm, I'm not getting the same diagnostic error. I wonder what's up

But, switching the code to

x() { return Promise.resolve(1).then(a => a + 1); }

Does give me a diagnostic error:

fileDiagnostics: [ { range: [Object], message: "Cannot find name 'x'.", severity: 1, code: 2304, source: 'typescript', relatedInformation: undefined } ] }

Which I can ignore by setting ignoredDiagnosticCodes: [2304], and that makes the tests pass

I'll edit the test for this then.

Maybe it has to do with the typescript version in use, or the target; I'm unclear as to which settings are used in the tests.

Pushed the test change on this fixup commit

prabirshrestha · 2020-11-20T18:26:06Z

seems like should pass as workspace config instead of commandline. or could support both

rchl · 2020-11-20T20:37:36Z

Good point IMO. And I would not introduce a command-line option for something like that. Command-line options should be reserved for toggling things that can't be changed once the server is started or are global in nature that can't be controlled with workspace settings.

pyrho · 2020-11-20T22:09:42Z

seems like should pass as workspace config instead of commandline. or could support both

Agreed that this would be better, does typescript-language-server already support such configuration option (via an external config file) ? If not I think this is outside the scope of this PR.
If yes, could you point me in the right direction?

Thanks.

rchl · 2020-11-21T22:03:44Z

seems like should pass as workspace config instead of commandline. or could support both

Agreed that this would be better, does typescript-language-server already support such configuration option (via an external config file) ? If not I think this is outside the scope of this PR.
If yes, could you point me in the right direction?

The client can provide settings through the params object to the initialize request.
It's handled here:
https://github.com/theia-ide/typescript-language-server/blob/fdf28313833cd6216d00eb4e04dc7f00f4c04f09/server/src/lsp-server.ts#L100-L104

rchl · 2020-11-21T22:08:53Z

(there is also workspace/configuration which is more dynamic and can be set in more granular way in IDEs but since that's not currently supported, it would be out of scope to add it.)

pyrho · 2020-11-30T12:33:30Z

Thank you @rchl, according to neovim's LSP doc this is doable, I will look into it when I get a chance.

prabirshrestha · 2020-12-11T16:29:13Z

vim-lsp supports it too. You can change it via vimrc.

rchl · 2021-10-18T19:48:04Z

I've re-implemented this PR in a slightly different way in #272 so closing this one. Thanks!

pyrho changed the title ~~[WIPfeat(diagnostics): ability to ignore some codes~~ [WIP] feat(diagnostics): ability to ignore some codes Nov 18, 2020

pyrho force-pushed the feat/ignore-diagnostics branch from 8cb903b to 4c240bd Compare November 18, 2020 20:44

rchl reviewed Nov 18, 2020

View reviewed changes

server/src/lsp-connection.ts Outdated Show resolved Hide resolved

feat(diagnostics): ability to ignore some codes

d1529af

pyrho commented Nov 18, 2020

View reviewed changes

pyrho force-pushed the feat/ignore-diagnostics branch from 4c240bd to d1529af Compare November 18, 2020 23:29

fixup! feat(diagnostics): ability to ignore some codes

c1666a2

DonnieWest mentioned this pull request Dec 11, 2020

New release? #177

Closed

rchl closed this Oct 18, 2021

[WIP] feat(diagnostics): ability to ignore some codes #172

[WIP] feat(diagnostics): ability to ignore some codes #172

Uh oh!

Conversation

pyrho commented Nov 18, 2020 • 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!

rchl Nov 19, 2020 • 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!

Choose a reason for hiding this comment

Uh oh!

DonnieWest Nov 20, 2020 • 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!

Choose a reason for hiding this comment

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!

prabirshrestha commented Nov 20, 2020

Uh oh!

rchl commented Nov 20, 2020

Uh oh!

pyrho commented Nov 20, 2020

Uh oh!

rchl commented Nov 21, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rchl commented Nov 21, 2020 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

pyrho commented Nov 30, 2020

Uh oh!

prabirshrestha commented Dec 11, 2020

Uh oh!

rchl commented Oct 18, 2021

Uh oh!

Uh oh!

pyrho commented Nov 18, 2020 •

edited

Loading

rchl Nov 19, 2020 •

edited

Loading

DonnieWest Nov 20, 2020 •

edited

Loading

rchl commented Nov 21, 2020 •

edited

Loading

rchl commented Nov 21, 2020 •

edited

Loading