Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add ability to override language_id per selector #1501

Closed
wants to merge 2 commits into from

Conversation

rchl
Copy link
Member

@rchl rchl commented Dec 1, 2020

Since we lost the ability to override the languageId with the
introduction of "selector" add the ability to override them.

This is a proof of concept in relation to sublimelsp/LSP-json#74.
I'm not sure we really want it and if we do, whether in that form.

It would be used with a configuration like:

    "language_id_overrides": {
        "source.json": "jsonc"
    },

in client config or a plugin.

Since we lost the ability to override the languageId with the
introduction of "selector" this adds ability to override them.
@rchl rchl changed the title Add ability to override langauge_id per-selector Add ability to override langauge_id per selector Dec 1, 2020
@predragnikolic
Copy link
Member

Not for it.

Why not override the langauge_id directly?

"json": {
+    "languageId": "jsonc",
    "enabled": true,
    "command": [...],
    "selector": "source.json"
}

@rchl
Copy link
Member Author

rchl commented Dec 1, 2020

Why not override the langauge_id directly?

Because it doesn't work like that (anymore).

And if you mean "why not implement it like that?" then that wouldn't be right because there can be multiple selectors and we wouldn't necessarily want to assign the same ID to all.

@predragnikolic
Copy link
Member

Because it doesn't work like that (anymore).
True

You can use languages in the client configuration to set the languageId to a document_selector.
But languages has been marked as deprecated.

languages = config.get("languages")

@rchl
Copy link
Member Author

rchl commented Dec 1, 2020

You can use languages in the client configuration to set the languageId to a document_selector.
But languages has been marked as deprecated.

I believe that even that is no longer possible in ST4. The language_id is only used to build the selector and so if you override it to jsonc, it will create a source.jsonc selector which will not match any view.

@rwols
Copy link
Member

rwols commented Dec 1, 2020

For some reason I cannot copy links from Discord, but we've had this discussion before :-) and the conclusion was to send json, and hope that people would write/maintain schemas with "allowComments": true.

sublimehq/Packages#285

Quoting from Discord:

sending jsonc for all json files would be too loose. You wouldn't get benefit of checking validity of trailing commas and comments

@rwols
Copy link
Member

rwols commented Dec 1, 2020

It would also be fine in my book to always send jsonc as the language ID. That is really what Packages/JSON/JSON.sublime-syntax represents.

@rchl
Copy link
Member Author

rchl commented Dec 1, 2020

I still believe that it would be bad to override lanague_id to jsonc for all files. I wouldn't use that personally. But since someone asked, we could provide an option at least, if someone wants to do it.

I think the proper solution would be to maintain a database of associations and then have something in the plugin API to override that per-file. But that's a separate topic.

@rwols
Copy link
Member

rwols commented Dec 1, 2020

By the way, some schemas do not include "allowComments": true at all, and expect a jsonc language ID. Quoting deathaxe from Discord because I just cannot seem to copy a link:

It seems to fail with Microsoft's Terminal settings. The Terminal App organizes settings in a settings.json which is located somewhere in %APPDATA%. It is opened in ST, but nothing happens in the first place. No server is started. By adding the containing folder to the sidebar the server is started but seems to apply ordinary whatever json schema, which marks all comments as errors. No completions are suggested as well.

@rchl
Copy link
Member Author

rchl commented Dec 1, 2020

By the way, some schemas do not include "allowComments": true at all, and expect a jsonc language ID. Quoting deathaxe from Discord because I just cannot seem to copy a link:

Because allowComments is a vscode-extension and can't be included in the official schemas downloaded from schema store.

The solution to sublimelsp/LSP-json#74 could be to inject allowComments and/or allowCommas to schemas that we fetch from schema store but then we'd need to know to which we should inject.

EDIT: Actually that wouldn't be that easy because we don't fetch the schema content, the server does...

@rchl
Copy link
Member Author

rchl commented Dec 2, 2020

And BTW, this is how VSCode knows whether it should use jsonc or json language ID: https://github.com/microsoft/vscode-eslint/blob/51da58ba4354de194d120d4da28b211033615a79/package.json#L432-L446

@rchl
Copy link
Member Author

rchl commented Dec 2, 2020

OK, I don't think we need this right now at least (maybe some use case will pop up in the feature).

I think what we need right now is a plugin API for overriding language ID per-file. Then LSP-json would maintain a database of "filepath => language id" and it would be queried before opening the file.

@rchl rchl changed the title Add ability to override langauge_id per selector Add ability to override language_id per selector Dec 2, 2020
@rchl rchl closed this Dec 2, 2020
@rwols
Copy link
Member

rwols commented Dec 2, 2020

I suppose I cannot convince you to send jsonc for everything ;) ? But yeah, it's just this one plugin that has problems. Better to have some sort of callback mechanism or something.

@rchl
Copy link
Member Author

rchl commented Dec 2, 2020

I suppose I cannot convince you to send jsonc for everything

No, because then I wouldn't see actual issues caused by trailing commas in plain json files like package.json. :)

@rchl rchl deleted the fix/id-override branch December 2, 2020 21:41
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants