Additional compiler vlty

[matze-dd]

This PR relates to Issue #1698, more specifically this post.

[lervag]

Thanks! I think this looks very good, although I do have some comments. A couple of general remarks:

• I prefer to use the dict.key notation instead of the dict['key'] notation, as I think it is in general easier to read.

• I prefer two spaces indentations for vimscript.

• I'm not sure if we'll be able to easily add an automatic test for this, but I would still prefer to add a test case to make it easy to manually test. One way would be to copy test/issues/template into test/issues/1714, then make a minimal set of test.vim and test.tex files that at least easily allows me/you/others to do nvim -u test.vim test.tex and then have a semi-sensible setup that is easy to use to test that things look ok.

I could do some changes myself to adjust to my preferred style, if you want, and if you give me push access to your fork.

[lervag]

* Is the name vlty OK? It tries to include the component names, but should not be too long.

Yes, but: I prefer to keep my options "classified" with topics, e.g. g:vimtex_{topic}_{name}. In this sense, I think a better name would be g:vimtex_grammar_vlty.

* Does the configuration via dictionary `g:vimtex_vlty` fit into vimtex's framework?

Yes, I think it looks good, but see my minor comments above.

* Does the manual section *compiler-vlty* cover all important points? Is it too verbose?

I think it looks very good. In particular, it seems to make it easy to follow the guide and make this work (I have not tested yet, but I will before merging).

* Does `:VimtexReload` in the vimrc below have negative side-effects? Is there a "more elegant" way?

I can't guarantee it does not have negative side-effects, but the intent is for it to work as expected.

Simple example

A vimrc could contain:

set spelllang=en_gb
let g:vimtex_vlty = {'ltdirectory': 'path/to/LanguageTool'}
map <F9> :w<CR>:VimtexReload<CR>:compiler vlty<CR>:make<CR>:cwindow<CR><CR>

Here, the :VimtexReload updates detected document classes and packages. The subsequent :compiler vlty includes these into the command invoked by :make.

I'm not sure that I want people to rely on VimtexReload this way. I would instead open a new issue/feature request to add a command to specifically update the detected document classes and packages (and apply possible consequences from this, e.g. syntax packages).

As suggested above: How about adding stuff like this in an example in test/issues/1714?

(EDIT: Since the LaTeX filter detects \documentclass and \usepackage on its own, this is only necessary for multi-file projects, where a single LaTeX file may lack this information.)

Note quite sure what you mean here.

With input ... we get on F9 the quickfix window ...

Very neat! I can see myself using this! Have you tested it yourself on large documents, in particular, does it perform well/fast?

[matze-dd]

Thank you for the detailed comments! I'll need some time to go through. Just one quick reply:

Very neat! I can see myself using this! Have you tested it yourself on large documents, in particular, does it perform well/fast?

Happy to hear that! But I can't say whether it will perform really well for all texts. Inevitably, it is somehow tailored to my own documents.
After cating all parts of a larger maths-oriented project into a single 700kB file, the filter itself takes 12 seconds on my 32-bit machine for this file. One cannot neglect the filtering time (a compiled language probably would be much faster), but it is always much shorter than the time consumed by LanguageTool.

[matze-dd]

I could do some changes myself to adjust to my preferred style, if you want, and if you give me push access to your fork.

All the changes you propose seem reasonable to me. It is probably best, if you make the adjustments. What do you mean with push access to my fork? Is it enough, if the box "Allow edits by maintainers" on the right margin at the top of this page is checked?

There is only one minor point: the current option names like 'disablecategories' agree with the names used by LanguageTool. If we use 'disable_categories', then this direct correspondence is lost. Prefixing all names with 'lt_' is sensible, of course.

I'm not sure if we'll be able to easily add an automatic test for this, but I would still prefer to add a test case to make it easy to manually test. One way would be to copy test/issues/template into test/issues/1714, then make a minimal set of test.vim and test.tex files that at least easily allows me/you/others to do nvim -u test.vim test.tex and then have a semi-sensible setup that is easy to use to test that things look ok.

Unfortunately, I currently do not have time to delve into, but I really would like to do that later. (For the YaLafi tests, I use a small Python HTTP server that mimics the LanguageTool server. This allows some interface tests without installing LanguageTool.) Would it be OK to postpone the test issue to a later point in time?

I'm not sure that I want people to rely on VimtexReload this way. I would instead open a new issue/feature request to add a command to specifically update the detected document classes and packages (and apply possible consequences from this, e.g. syntax packages).

This agrees with my slight worry about VimtexReload. It seems to be a bit "too heavy", here.

(EDIT: Since the LaTeX filter detects \documentclass and \usepackage on its own, this is only necessary for multi-file projects, where a single LaTeX file may lack this information.)

Note quite sure what you mean here.

Reading the vimtex variables, the compiler interface passes detected packages and classes as command-line arguments to the LaTeX filter. This is in fact redundant (and should not harm), if the declarations are located in the LaTeX file that is just being checked. The filter detects them, too.

[lervag]

Ok, I've implemented the changes I've suggested, but I'm not able to push my changes to your branch. I don't know exactly how to do it. But I could also just merge the changes into master, as there are no breaking changes. Then we could continue the discussion here, and if necessary, you could open a new PR to follow up?

[lervag]

Happy to hear that! But I can't say whether it will perform really well for all texts. Inevitably, it is somehow tailored to my own documents.
After cating all parts of a larger maths-oriented project into a single 700kB file, the filter itself takes 12 seconds on my 32-bit machine for this file. One cannot neglect the filtering time (a compiled language probably would be much faster), but it is always much shorter than the time consumed by LanguageTool.

Sounds reasonable. I'll just have to test and get a feel for it.

There is only one minor point: the current option names like 'disablecategories' agree with the names used by LanguageTool. If we use 'disable_categories', then this direct correspondence is lost. Prefixing all names with 'lt_' is sensible, of course.

Good point. I landed on the lt_enablecategories form.

I'm not sure if we'll be able to easily add an automatic test for this, but I would still prefer to add a test case to make it easy to manually test. One way would be to copy test/issues/template into test/issues/1714, then make a minimal set of test.vim and test.tex files that at least easily allows me/you/others to do nvim -u test.vim test.tex and then have a semi-sensible setup that is easy to use to test that things look ok.

Unfortunately, I currently do not have time to delve into, but I really would like to do that later. (For the YaLafi tests, I use a small Python HTTP server that mimics the LanguageTool server. This allows some interface tests without installing LanguageTool.) Would it be OK to postpone the test issue to a later point in time?

Yes, that's OK! But I would be happy to see a new PR or update for this when/if you get the time.

[matze-dd]

Ok, I've implemented the changes I've suggested, ...

Thank you!

But I could also just merge the changes into master, as there are no breaking changes. Then we could continue the discussion here, and if necessary, you could open a new PR to follow up?

Agreed, this is probably the simplest way for now. We will have to improve our expertise in using GitHub, if we run again into a similar situation ;-).

[lervag]

I don't understand why the PR was closed, but it does not matter. I've pushed my changes to your branch now, so please take a look.

[lervag]

Sorry, my bad. I screwed up the push. I'll fix it.

(Note: No worries, I also have everything local, so things will work out properly when I figure it out.)

[lervag]

There, it is fixed now. I've (force) pushed my updates to your fork. I accidentely created a new branch that you may disregard (delete, if you want). On your side, you should do the following in your local forked repo:

git fetch
git co master
git reset --hard origin/master

This will fetch the latest branch from github, then (forcefully) reset your local master to the new origin master. This is necessary because of the force push I've used to rebase your changes on top of the latest vimtex master.

[matze-dd]

Amazed. All seems to be perfect now.

I need to admit that I do not maintain a local git repo. As my few projects have been that small till now, I always used the simple file uploading mechanism of GitHub's GUI.

Thank you so far! I will have a look at the changes.

[lervag]

Amazed. All seems to be perfect now.

Great!

I need to admit that I do not maintain a local git repo. As my few projects have been that small till now, I always used the simple file uploading mechanism of GitHub's GUI.

Yes, that probably also works well. :)

Thank you so far! I will have a look at the changes.

Great.

[matze-dd]

Just corrected three typos in vimtex.txt.

For vlty.vim, I have a remark concerning the installation test. The idea with printing an error message directly to the user was stolen from textidote.vim. In your new version, :compiler vlty just goes through, and :make complains about missing a makfile, if the installation test fails.
I think, we should print a hint to the user that something went wrong. My proposal is to use the same descriptive message as written to the log.

[lervag]

Just corrected three typos in vimtex.txt.

Great, good catch!

For vlty.vim, I have a remark concerning the installation test. The idea with printing an error message directly to the user was stolen from textidote.vim. In your new version, :compiler vlty just goes through, and :make complains about missing a makfile, if the installation test fails.
I think, we should print a hint to the user that something went wrong. My proposal is to use the same descriptive message as written to the log.

The vimtex#log#error facility should give the error messages, though. That's the point of this function. If the error message is not displayed, than that's a bug and we should discuss it. I don't see a reason to add the echoerrs in addition.

[lervag]

On my end, the error message from Yalafi is displayed as expected. It is not as "intruding" as the echoerr, but if we add a line of output, then it will be. E.g.:

  call vimtex#log#error([
        \ 'vlty compiler requires Python',
        \ 'Please see ":help vimtex-vlty" for more details.',
        \])

[matze-dd]

First of all, a (late) Thank you! for correcting mistakes and reducing complexity in vlty.vim. (For instance, I did not know the subtleties of string comparison operators.)

On my end, the error message from Yalafi is displayed as expected.

OK, I could find the reason why it sometimes isn't shown. If one says :compiler vlty | lmake as in the documentation example, then the message is dropped. So, should we add something like (the first sentence is already in the doc)?:

The given directory has to contain the `LanguageTool` software, including for
instance the file `languagetool-server.jar`. A separated `:compiler vlty` will
raise an error message, if some component cannot be found.

Including your proposal from the last post, I have reverted the changes to vlty.vim.

EDIT. added 'separated'

[lervag]

OK, I could find the reason why it sometimes isn't shown. If one says :compiler vlty | lmake as in the documentation example, then the message is dropped. So, should we add something like (the first sentence is already in the doc)?:

Thanks, I think your update is good. And I agree, it seems appropriate to make this comment in the docs. Will you update, or should I?

I think this is ready for merge now. It remains for me to actually test it. I'll do that before I merge. I'll follow the docs and install things and see if it works as expected. Sounds good?

[matze-dd]

Will you update, or should I?

Just added.

It remains for me to actually test it. I'll do that before I merge. I'll follow the docs and install things and see if it works as expected. Sounds good?

Yes, it does! Touching wood ...

[lervag]

Ok, I've tested now. A couple of comments:

• If I install LanguageTool in Arch Linux (sudo pacman -S languagetool), then the file languagetool-server.jar is put into /usr/share/java/languagetool. If I use this directory as the lt_directory, then I get the following error when I try to run the compiler:

  Error: Unable to initialize main class org.languagetool.commandline.Main
  Caused by: java.lang.NoClassDefFoundError: org/languagetool/rules/Rule
  *** yalafi.shell: warning:
  *** could not load module 'yalafi.documentclasses.article'
  *** yalafi.shell: warning:
  *** could not load module 'yalafi.documentclasses.article'
  
  *** yalafi.shell: internal error:
  error reading JSON output from proofreader, (sub-)item "JSON root element"
  

• If I follow the directions in the help, it seems to work. But the command creates a lot of "noise". It also has the warning about yalafi.documentclasses.article (and a lot of warnings about packages and stuff, like could not load module 'yalafi.packages....). Perhaps that's OK. I think this may be one of the points of plugins like vim-dispatch.

• I think it would be good to add a type to the messages (e.g. warning or information).

• When I test this on a larger document, it does take some time. I don't think that's a big problem, though.

• There's a lot of false positives, e.g. text in \cite*{...}, text in macros in the preamble, and similar. Not a big problem, either, but still somewhat annoying.

• For some reason, it does not work to open a particular warning with <cr> inside the quickfix window. I suspect it is because the entries do not have the file attribute. When I inspect the output, I see this:

  === paper-ebus.tex ===
  22.) Line 54, column 17, Rule ID: UNIT_SPACE[1]
  Message: ...
  

  I'm not sure if this output comes from Yalafi, but: Is it possible to combine the first two lines to make it easy to also parse the file name?

  If not, then I think it is possible to modify the errorformat to first parse the filename itself.

[matze-dd]

Ok, I've tested now.

Thanks a lot!

If I install LanguageTool in Arch Linux (sudo pacman -S languagetool), then the file languagetool-server.jar is put into /usr/share/java/languagetool. If I use this directory as the lt_directory, then I get the following error when I try to run the compiler:

I will check that.

If I follow the directions in the help, it seems to work. But the command creates a lot of "noise". It also has the warning about yalafi.documentclasses.article (and a lot of warnings about packages and stuff, like could not load module 'yalafi.packages....). Perhaps that's OK. I think this may be one of the points of plugins like vim-dispatch.

The warning for standard document classes will vanish with next YaLafi version on PyPI (currently, only scrreprt is included). For packages, I'm undecided. It will be definitely impossible to include all packages into YaLafi. If a user activates an unknown package, then probably some macros won't be treated correctly. Therefore, a warning might be appropriate. On the other hand, this may be annoying if too many warnings are raised. I could add an option to suppress these.

I think it would be good to add a type to the messages (e.g. warning or information).

Good point. Actually, the 'warning:' type currently is printed on the line before. I will adjust that.

When I test this on a larger document, it does take some time. I don't think that's a big problem, though.

The main run time stems from LanguageTool. Here are two directions one could check in the future.

• Vim plugin ALE does the check in the background. I have not yet tested that for larger documents.
• The project vscode-ltex seems to use an internal sentence cache, such that only changed parts of a large document under work need to be re-checked. LanguageTool itself has a quite sophisticated machinery to detect sentence boundaries. So, it might be a better idea to cache paragraphs instead.

There's a lot of false positives, e.g. text in \cite*{...}, text in macros in the preamble, and similar. Not a big problem, either, but still somewhat annoying.

Thanks for spotting that. Macros like \cite* need to be included in next YaLafi version. Do you have more of these standard examples (together with package, if from one)? For the noise from the preamble, one could go the way of TeXtidote. Here, only the LaTeX code starting with \begin{document} is checked by default. This is obviously useful if the "real text" of a project is not placed in separate files.

I'm not sure if this output comes from Yalafi, but: Is it possible to combine the first two lines to make it easy to also parse the file name?
If not, then I think it is possible to modify the errorformat to first parse the filename itself.

Yes, this is from YaLafi. I will try the second version.

[lervag]

Therefore, a warning might be appropriate. On the other hand, this may be annoying if too many warnings are raised. I could add an option to suppress these.

Actually, I like to avoid options. There are already very many options in vimtex. But I'm not sure what's best here, though. It's too hard to cover all packages, yet seems sensible to warn about missing package support. Still, perhaps not a problem that the warnings are visible?

At least, I don't think this is a blocker, so let's keep it as it is for now.

Do you have more of these standard examples (together with package, if from one)?

Yes: hyperref has \url and \href, as well as some cite commands. biblatex has a lot of cite commands. tikz and pgfplots would be nice to have support for. Probably a lot more, but again, this is not a blocker for the current PR.

For the noise from the preamble, one could go the way of TeXtidote.

I think that sounds like a good idea. Or, perhaps one could specifically look for common things in the preamble, like the \title{...} macro?

---

There are some updates pending, I hope it's OK for you that we postpone the merge until those are in:

• Parse filename in errorformat
• More specific entry types

The other things are all minors and can be considered tangent improvements.

[matze-dd]

At least, I don't think this is a blocker, so let's keep it as it is for now.

D'accord.

Yes: hyperref has \url and \href, as well as some cite commands. biblatex has a lot of cite commands. tikz and pgfplots would be nice to have support for. Probably a lot more, but again, this is not a blocker for the current PR.

Thanks. I will put these on the TODO list for YaLafi.

I think that sounds like a good idea. Or, perhaps one could specifically look for common things in the preamble, like the \title{...} macro?

This is one of my concerns, too (\title is already covered). Will have to think this over to find an "elegant" solution. But agreed, this is for "Further work."

There are some updates pending, I hope it's OK for you that we postpone the merge until those are in ...
The other things are all minors and can be considered tangent improvements.

This is OK for me, too. What do you mean with "more specific entry types"?

[lervag]

This is OK for me, too. What do you mean with "more specific entry types"?

Simply to use %E, %W, or %I instead of %A in the errorformat.

[lervag]

Any updates here?

[matze-dd]

Sorry for the long delay; currently a lot of trouble in the "real world".

I have just added the file name and entry type %I (as it takes the least space in the status line) to errorformat .

[lervag]

Sorry for the long delay; currently a lot of trouble in the "real world".

"Real world" >> "vimtex", so no problem! I hope you are well/that the trouble is getting less.

I have just added the file name and entry type %I (as it takes the least space in the status line) to errorformat .

Thanks! With that, I think things are ready for merge, and so I've merged this. I think this brings a useful feature to vimtex, so again, thanks!

[matze-dd]

"Real world" >> "vimtex", so no problem! I hope you are well/that the trouble is getting less.

Thanks, it is becoming better, fortunately.

Thanks! With that, I think things are ready for merge, and so I've merged this. I think this brings a useful feature to vimtex, so again, thanks!

Thank you for the merge! I will try to add the things we have discussed later.

[lervag]

Thanks, it is becoming better, fortunately.

Good to hear!

Thanks! With that, I think things are ready for merge, and so I've merged this. I think this brings a useful feature to vimtex, so again, thanks!

Thank you for the merge! I will try to add the things we have discussed later.

Great :)

[matze-dd]

I got a notice on your discussion (@lervag and @Konfekt) concerning the problem with python3, but could not find the place; only have the reference to fix 224f912.

Maybe, we could make it even more robust by checking the Python version? The rationale is that apparently a lot of systems still have Python version 2 installed by default as python. It could look like

call system(s:python . ' -c "import sys; assert sys.version_info.major >= 3 and sys.version_info.minor >= 6")
if v:shell_error != 0
  call s:installation_error('vlty compiler requires at least Python version 3.6')
  finish
endif

Should I make a PR?

[lervag]

Maybe, we could make it even more robust by checking the Python version? The rationale is that apparently a lot of systems still have Python version 2 installed by default as python. It could look like ...

Should I make a PR?

I assume you already noticed that I pushed an update for this. I agree it could be even more robust by adding something like what you propose. Feel free to make a PR! My immediate thought is: try and keep the lines at 80 cols, else it looks good. :)

[lervag]

I got a notice on your discussion (@lervag and @Konfekt) concerning the problem with python3, but could not find the place; only have the reference to fix 224f912.

If you get the same kind of email notification as I do, then you can follow the link in the email to the specific commit with a diff of the file. Then you can scroll down to find the comments, which are related to a specific line (or lines).

[matze-dd]

If you get the same kind of email notification as I do, then you can follow the link in the email to the specific commit with a diff of the file. Then you can scroll down to find the comments, which are related to a specific line (or lines).

Thanks for this. <unimportant> But somehow, I'm still at the beginning of the learning curve: the notification mail only included the link to the fix (no diff), and at the page of the fix, there are no comments. </unimportant>

My immediate thought is: try and keep the lines at 80 cols, else it looks good. :)

Yes, this was just typed in directly here, and the closing quote ' of the string was missing, too. :)

[lervag]

But somehow, I'm still at the beginning of the learning curve: the notification mail only included the link to the fix (no diff), and at the page of the fix, there are no comments.

My initial notification email looked something like this:

Could be checked for `python3` instead of `python`? On openSUSE 15.2, released this month, `python` defaults to version `2.7` incompatible with `yalafi`.

Thank you for adding this compiler!

---

You are receiving this because you authored the thread.
Reply to this email directly, view it on GitHub, or unsubscribe.

If you follow the link "view it on GitHub", it will open the commit information. To see the comments, scroll down to line 63.

Hope this helps!

[matze-dd]

If you follow the link "view it on GitHub", it will open the commit information. To see the comments, scroll down to line 63. Hope this helps!

Thank you for the time! Me stupid. The link was in the email and I went there, but only looked at beginning and end of the page, which is both Python code.

[lervag]

No problem, happy to help :)

[matze-dd]

Hi,

as I cannot engage in maintenance as necessary, @torik42 has volunteered to take over further development of YaLafi, see the corresponding YaLafi issue.

The repository now is https://github.com/torik42/YaLafi, but the links to matze-dd/YaLafi continue to work. A browser is automatically redirected to the new location. An archived repository of version 1.3.1 is available as matze-dd/YaLafi-1.3.1.

Thank you again for integrating the interface into VimTeX! Looking at the list of fixed related issues, at least some people seem to use it ;)

Best whishes,
Matthias

[lervag]

Hi,

as I cannot engage in maintenance as necessary, @torik42 has volunteered to take over further development of YaLafi, see the corresponding YaLafi issue.

The repository now is https://github.com/torik42/YaLafi, but the links to matze-dd/YaLafi continue to work. A browser is automatically redirected to the new location. An archived repository of version 1.3.1 is available as matze-dd/YaLafi-1.3.1.

Thank you again for integrating the interface into VimTeX! Looking at the list of fixed related issues, at least some people seem to use it ;)

Best whishes, Matthias

Thank you, Matthias, for building and sharing useful tools for the LaTeX community! It was only a pleasure working with you; I don't really think I did very much here - you provided all the necessary code through PRs, I only helped form it according to my (probably partly silly) code and documentation style.

Also, thank you @torik42 for taking over the development and maintenance. Please feel free to let me know if I should make changes or updates to VimTeX (code and/or docs) as a result of updates to YaLafi!