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

Compiler vlty: support LanguageTool as installed by pacman #1821

Closed
wants to merge 5 commits into from
Closed

Compiler vlty: support LanguageTool as installed by pacman #1821

wants to merge 5 commits into from

Conversation

matze-dd
Copy link
Contributor

@matze-dd matze-dd commented Oct 7, 2020

This PR addresses an issue raised in the discussion of vlty. Under Arch Linux, one now can use LanguageTool as installed with

pacman -S languagetool

The minimal vimrc is given in the documentation.
This requires YaLafi version 1.1.5. It has been uploaded to PyPI, such that it is available via pip.

@@ -51,9 +57,12 @@ let s:language = substitute(s:language, '_', '-', '')

let &l:makeprg =
\ s:python . ' -m yalafi.shell'
\ . ' --lt-directory ' . s:vlty.lt_directory
\ . ' --lt-command "' . s:vlty.lt_command . '"'
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Does it make sense to always supply the --lt-command option, or should this only be supplied if lt_command is available?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, this is just the version with the least key strokes. I will change that.

@@ -1616,6 +1616,8 @@ OPTIONS *vimtex-options*

lt_directory~
Path to the `LanguageTool` software.
lt_command~
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like it better if there's an empty line between the descriptions. Also, I'm curious if lt_command perhaps should be the first entry here?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I like it better if there's an empty line between the descriptions.

OK, I will insert a blank line.

Also, I'm curious if lt_command perhaps should be the first entry here?

Don't know, which key is more important. But I assume that most people will install LanguageTool "manually".

doc/vimtex.txt Outdated
instance the file `languagetool-server.jar`. A separated `:compiler vlty` will
raise an error message, if some component cannot be found.
instance the file `languagetool-server.jar`.
If `LanguageTool` has been installed via a packet manager as given above,
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I prefer the paragraphs to be full "blocks" of text, so move If ... back to the previous line. If you meant to start a new paragraph, then add an empty line.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

OK.

finish
endif
else
if !filereadable(fnamemodify(s:vlty.lt_directory
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think it is better to use elseif here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

My first version was this way, and it is in fact synonymous here. But then, I thought the current code would be more readable. I will change that back.

@lervag
Copy link
Owner

lervag commented Oct 7, 2020

This PR addresses an issue raised in the discussion of vlty. Under Arch Linux, one now can use LanguageTool as installed with

pacman -S languagetool

The minimal vimrc is given in the documentation.
This requires YaLafi version 1.1.5. It has been uploaded to PyPI, such that it is available via pip.

Great, thanks! I appreciate the update, I only had a few minor comments!

@matze-dd
Copy link
Contributor Author

matze-dd commented Oct 7, 2020

Great, thanks! I appreciate the update, I only had a few minor comments!

My thanks go back for your great project :)

I will incorporate the changes and make a new comment afterwards.

@matze-dd
Copy link
Contributor Author

matze-dd commented Oct 7, 2020

The changes are included. Only for the order of lt_directory and lt_command, things are kept. I think, most people will use the manual LanguageTool installation, although this is only a guess.

@lervag
Copy link
Owner

lervag commented Oct 7, 2020

Thanks! I merging this now.

lervag added a commit that referenced this pull request Oct 7, 2020
@lervag lervag closed this Oct 7, 2020
@lervag
Copy link
Owner

lervag commented Oct 7, 2020

The changes are included. Only for the order of lt_directory and lt_command, things are kept. I think, most people will use the manual LanguageTool installation, although this is only a guess.

Seems sensible. Again, thanks! :)

@matze-dd
Copy link
Contributor Author

matze-dd commented Oct 8, 2020

Thank you for merging and the final adjustments! These are all improvements.

@lervag
Copy link
Owner

lervag commented Oct 8, 2020

My pleasure :)

lervag added a commit that referenced this pull request Oct 14, 2020
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.

2 participants