interperet `nothin` -> false in client config response handling #696

ZacLN · 2020-05-13T06:32:57Z

julia-vscode/julia-vscode#1184 (comment)
This ignores the SL/DF-Options constructors that have been recently - ths seems like an LSP specific interface so shouldn't really sit in those packages.

davidanthoff

Looks good to me, @non-Jedi, do you also want to take a look?

non-Jedi · 2020-05-13T11:57:55Z

src/requests/workspace.jl

@@ -103,9 +103,8 @@ function request_julia_config(server::LanguageServerInstance)
        ]))

    server.format_options = DocumentFormat.FormatOptions(response[1:11]...)
-    server.runlinter = something(response[22], true)
+    server.runlinter = something(response[22], false)


Well crap. Is it just this setting that vscode sends null for false on, or is it all of them? I'll go reread the spec, but this smells like a spec-violation on the part of vscode.

non-Jedi · 2020-05-13T12:05:31Z

Per the spec, a null value means that the client is unable to provide a setting which is semantically different from providing a false value.

If the client can’t provide a configuration setting for a given scope then null need to be present in the returned array.

Also note that this change by itself doesn't solve julia-vscode/julia-vscode#1184 since that issue is about "julia.lint.call" rather than "julia.lint.run".

So:

Is this a reported, known issue on the vs-code side?
Is there no way to workaround this in the vs-code plugin itself? It would be really nice if we didn't have to hurt spec-compliant clients because vs-code isn't complying with the spec.

non-Jedi · 2020-05-13T12:10:55Z

One option that wouldn't break defaults for non-vscode clients would be to change all boolean config fields to be Ints instead with any value other than 0 being equivalent to true.

ZacLN · 2020-05-13T15:34:05Z

This still needs to be verified by another vscode user I think.

ZacLN · 2020-05-13T16:02:48Z

Also note that this change by itself doesn't solve julia-vscode/julia-vscode#1184 since that issue is about "julia.lint.call" rather than "julia.lint.run".

Thanks - I hadn't pushed a commit

non-Jedi · 2020-05-13T16:43:38Z

This still needs to be verified by another vscode user I think.

Are you unable to replicate it locally?

ZacLN · 2020-05-13T17:06:12Z

I see this behaviour on my vscode setup, not sure anyone else has confirmed it on theirs? (specifically the returning of nulls instead of false)

ZacLN · 2020-05-18T18:10:19Z

I'd rather we changed it to "on"/"off" to be a bit more transparent - is that ok with you?

non-Jedi · 2020-05-18T18:38:47Z

That's fine with me.

davidanthoff · 2020-05-18T20:50:35Z

I'd rather we changed it to "on"/"off" to be a bit more transparent

Should we maybe first check with the MS guys whether there is some other way around this? If our current understanding of the situation is correct, I agree that "on/off" is a better option, but this is frustrating that one can't just use a bool and be done with it... Maybe we could even convince them to just change the implementation and fix this?

Changing all of these settings to "on/off" is also annoying because every user who adjusted those settings will have to update...

marius311 · 2020-05-20T18:49:06Z

Just want to confirm that for me this PR fixes the inability to turn linting off that I mentioned on Slack and is mentioned in julia-vscode/julia-vscode#1184.

Would be great to get a fix in if possible, since this is a real "paper cut" bug I think alot of new users would probably hit.

ZacLN · 2020-05-21T15:59:20Z

OK, this definitely looks like a bug on the vscode side of things. I'm going to close this and put forward a pair of PRs for LanguageServer and julia-vscode that would move these configurations from bools to "on"/"off".

This feels like something that should be addressed rather quickly and I don't have a sense of how quickly changes would make their way through upstream. It's then an easy change to revert back when the bug is addressed. Thoughts?

non-Jedi · 2020-05-21T16:38:15Z

@ZacLN why not just use this branch to do a release for vscode (and fix the obviously super obnoxious ux of not being able to turn off configuration options) and then wait and see if the upstream bug is quickly fixed before you next want to do a real LanguageServer.jl release? That way we potentially avoid people having to redo their config.

ZacLN · 2020-05-21T17:33:48Z

I'm amenable to that, @davidanthoff ?

davidanthoff · 2020-05-23T02:12:54Z

Yes, I would much prefer that we stick with bool and true/false, and my understanding is that this PR here works around the bug in VS Code, right? So I would go with that strategy for now.

Normally up stream is super fast in addressing stuff. I'm sure if we were to open a PR ourselves that fixes the problem, it could be shipped very quickly. Also, because this is a bug in the vscode-languageclient npm package, we might not even have to wait for a VS Code release, I think they can release a bug fix release of that package independently from VS Code releases.

Also very much agree that we should ship this ASAP!

davidanthoff

Let’s merge this, just to fix this for now and so that we can push out a new release soon.

ZacLN · 2020-05-24T18:21:18Z

I think @non-Jedi's proposal was that we use this branch in the snapshot for a julia-vscode release rather than merging this

davidanthoff · 2020-05-24T22:17:42Z

I don’t think we can ship a branch, there is a fair bit of tooling in place on the extension side that assumes we ship master, and with our regular releases it would be painful to work around that.

Can we put the content of this branch behind an if clause that checks InitializeParams. clientInfo.name so that we only use this implementation if the client is VS Code? Presumably VS Code sends that field?

But in any case, we should ship something ASAP, right now users on the released version can’t change any of these settings and we also have crash reports because somewhere there is a call to a method with the wrong number of args in the currently shipping version.

@ZacLN let me know if you want me to do something about this PR if you are busy!

ZacLN · 2020-05-24T22:43:12Z

OK, I've amended as above

davidanthoff

Yeah!

interperet nothin -> false in client config response handling

eb0c901

ZacLN added the bug label May 13, 2020

ZacLN requested a review from davidanthoff May 13, 2020 06:32

davidanthoff added this to the Current milestone May 13, 2020

davidanthoff previously approved these changes May 13, 2020

View reviewed changes

non-Jedi reviewed May 13, 2020

View reviewed changes

others

80773be

ZacLN dismissed davidanthoff’s stale review via 80773be May 13, 2020 16:02

Merge branch 'master' into configfix

73becec

davidanthoff added 2 commits May 22, 2020 19:16

Merge branch 'master' into configfix

c471dd3

Merge branch 'master' into configfix

0cd34f9

davidanthoff previously approved these changes May 23, 2020

View reviewed changes

davidanthoff assigned ZacLN May 24, 2020

special handler for vscode

1dd9060

ZacLN dismissed davidanthoff’s stale review via 1dd9060 May 24, 2020 22:42

davidanthoff approved these changes May 24, 2020

View reviewed changes

davidanthoff merged commit 9e8c24c into master May 24, 2020

davidanthoff deleted the configfix branch May 24, 2020 23:03

pfitzseb mentioned this pull request Feb 10, 2021

Misclassification of client name #884

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

interperet `nothin` -> false in client config response handling #696

interperet `nothin` -> false in client config response handling #696

ZacLN commented May 13, 2020 •

edited

Loading

davidanthoff left a comment

non-Jedi May 13, 2020

non-Jedi commented May 13, 2020

non-Jedi commented May 13, 2020

ZacLN commented May 13, 2020

ZacLN commented May 13, 2020

non-Jedi commented May 13, 2020

ZacLN commented May 13, 2020

ZacLN commented May 18, 2020

non-Jedi commented May 18, 2020

davidanthoff commented May 18, 2020

marius311 commented May 20, 2020

ZacLN commented May 21, 2020

non-Jedi commented May 21, 2020 •

edited

Loading

ZacLN commented May 21, 2020

davidanthoff commented May 23, 2020

davidanthoff left a comment

ZacLN commented May 24, 2020

davidanthoff commented May 24, 2020

ZacLN commented May 24, 2020

davidanthoff left a comment

interperet nothin -> false in client config response handling #696

interperet nothin -> false in client config response handling #696

Conversation

ZacLN commented May 13, 2020 • edited Loading

davidanthoff left a comment

Choose a reason for hiding this comment

non-Jedi May 13, 2020

Choose a reason for hiding this comment

non-Jedi commented May 13, 2020

non-Jedi commented May 13, 2020

ZacLN commented May 13, 2020

ZacLN commented May 13, 2020

non-Jedi commented May 13, 2020

ZacLN commented May 13, 2020

ZacLN commented May 18, 2020

non-Jedi commented May 18, 2020

davidanthoff commented May 18, 2020

marius311 commented May 20, 2020

ZacLN commented May 21, 2020

non-Jedi commented May 21, 2020 • edited Loading

ZacLN commented May 21, 2020

davidanthoff commented May 23, 2020

davidanthoff left a comment

Choose a reason for hiding this comment

ZacLN commented May 24, 2020

davidanthoff commented May 24, 2020

ZacLN commented May 24, 2020

davidanthoff left a comment

Choose a reason for hiding this comment

interperet `nothin` -> false in client config response handling #696

interperet `nothin` -> false in client config response handling #696

ZacLN commented May 13, 2020 •

edited

Loading

non-Jedi commented May 21, 2020 •

edited

Loading