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

Support class and style files #53

Closed
LDAP opened this issue Mar 22, 2021 · 8 comments · Fixed by #54
Closed

Support class and style files #53

LDAP opened this issue Mar 22, 2021 · 8 comments · Fixed by #54
Assignees
Labels
1-feature-request ✨ Issue type: Request for a desirable, nice-to-have feature 3-fixed Issue resolution: Issue has been fixed on the develop branch
Milestone

Comments

@LDAP
Copy link
Contributor

LDAP commented Mar 22, 2021

Is your feature request related to a problem? Please describe.
LTEX-LS reports a lot of false positives in cls and sty files.

Describe the solution you'd like
Support cls and sty files.

Related question
Does LTEX-LS distinguish filetypes over languageId or by file extension? Is LaTex filtering of commands disabled if the languageId (or extension) does not match?

@LDAP LDAP added the 1-feature-request ✨ Issue type: Request for a desirable, nice-to-have feature label Mar 22, 2021
@LDAP
Copy link
Contributor Author

LDAP commented Mar 24, 2021

If I interpret these lines correctly:

@Nullable String codeLanguageId = (arguments.has("codeLanguageId")
? arguments.get("codeLanguageId").getAsString() : null);

if (fileNameStr.endsWith(".bib")) {
codeLanguageId = "bibtex";
} else if (fileNameStr.endsWith(".tex")) {
codeLanguageId = "latex";
} else if (fileNameStr.endsWith(".md")) {
codeLanguageId = "markdown";

LTEX-LS should first decide on languageId and if that fails using file extension? But I am curious why codeLanguageId instead of languageId is checked, since the LSP specification does only specify a languageId. The sublime client sends the language id according to the lines in LTEX-LS (on textDocument/didOpen) but LTEX-LS ignores those and uses extension instead.

@valentjn
Copy link
Owner

valentjn commented Mar 26, 2021

Language servers use the language ID of the text document when they “[handle] more than one language to avoid re-interpreting the file extension” (see LSP spec). So LTEX LS does this as well. Remember that the document URL may just be something like untitled:Untitled-1. You can't guess which language such files would have.

The code you quote is from a function called executeCheckDocumentCommand. This code is only run if the ltex.checkDocument command is executed, so when a manual check is requested by the client by executing this custom command (I think LSP plans to support this natively). codeLanguageId is a custom argument for this custom command; there is no languageId argument in this context (see the linked docs).

LTEX LS's code tries to distinguish whenever it can between the code language (LATEX, Markdown, etc.) and the (natural) language (English, German, etc.) of the document. Classic language servers deal with only one layer of language, as they usually don't have to bother with natural languages; therefore, this terminology is not present in the LSP spec. When I say code language ID in the following, I mean languageId in terms of the LSP spec.

When a file is checked through the usual way, i.e., automatic checks triggered by textDocument/didOpen and textDocument/didChange, LTEX LS does use only the code language ID of the document and not the file extension, see LtexTextDocumentService.didOpen and the constructor of LtexTextDocumentItem; the code language ID is ultimately used in DocumentChecker.fragmentizeDocument. If it's not working for you, then maybe you're triggering the checks manually via ltex.checkDocument, or it's a bug in either your client or the server.

Regarding the actual feature request, I'm not sure what the proposed solution should look like. If you tell LTEX LS that a class/style file is latex via its code language ID, then it will check it like every other LATEX file, so this is obviously working as expected. Do you want to reduce the number of false positives? If yes, which false positives do you mean?

Regarding the second related question, if the code language ID is not latex, then yes, LTEX LS won't parse LATEX commands. It's fully possible (though unreasonable) to parse a document.tex document as Markdown by setting the code language ID to markdown.

@LDAP
Copy link
Contributor Author

LDAP commented Mar 26, 2021

I may have found the issue in my side. The ST client sends tex as languageId instead of latex when dealing with cls and sty files.

@LDAP
Copy link
Contributor Author

LDAP commented Mar 26, 2021

I found the issue: The ST client indeed sends the languageId tex. This is the valid id according to https://microsoft.github.io/language-server-protocol/specification#textDocumentItem

@valentjn
Copy link
Owner

valentjn commented Apr 3, 2021

I thought the problem of this issue was that there were too many false positives with class and style files (see issue description)? Now it sounds like there were no diagnostics at all due to the language ID mismatch? Can you please clarify what exactly the problem of this issue is, and what the desired solution should look like?

I doubt that we should check class or style files by default, as they usually consist mostly of TEX code (maybe with a little bit of LATEX in it) with hardly any natural text, e.g., this example. That's why tex is the correct language ID for these files, and that's also why LTEX doesn't check these, as there would be lots of false positives as LTEX is not a full TEX parser. If the goal of this issue (which I don't know) was to enable checking these files, could you give an example class or style file that should be checked?

@LDAP
Copy link
Contributor Author

LDAP commented Apr 3, 2021

It's the languageId mismatch. The false positives were a result of that. In my opinion I would at least allow TeX files to be parsed and not handle the id tex as plain.
The server can be independently disabled by the client for TeX.

@valentjn valentjn self-assigned this Apr 5, 2021
@valentjn valentjn added this to the 11.0.0 milestone Apr 5, 2021
@valentjn
Copy link
Owner

valentjn commented Apr 5, 2021

Ok. I'm still not convinced it's a good idea to check these files, but it doesn't hurt to let the client decide by adding "tex" to ltex.enabled.

@valentjn valentjn added the 3-fixed Issue resolution: Issue has been fixed on the develop branch label Apr 5, 2021
@valentjn
Copy link
Owner

valentjn commented Apr 5, 2021

Fix released in 11.0.0.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
1-feature-request ✨ Issue type: Request for a desirable, nice-to-have feature 3-fixed Issue resolution: Issue has been fixed on the develop branch
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants