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

linter/markdown: adds support for languatool #2155

Merged
merged 14 commits into from
Mar 9, 2019
Merged

linter/markdown: adds support for languatool #2155

merged 14 commits into from
Mar 9, 2019

Conversation

wahrwolf
Copy link
Contributor

As requested in #1516 and #2154, this will add support for the super awesome grammar/style checker languagetool

I provide this without any tests

Let me know if you have any feedback!

@oblitum
Copy link
Contributor

oblitum commented Dec 21, 2018

The propaganda is so strong on those issues that I'm trying it now.

@oblitum
Copy link
Contributor

oblitum commented Dec 21, 2018

Location list position for me seems off, using example from #1516. How did it go for you?

@oblitum
Copy link
Contributor

oblitum commented Dec 21, 2018

Columns are right, lines are not.

@oblitum
Copy link
Contributor

oblitum commented Dec 21, 2018

In fact, I used LanguageTool from https://github.com/rhysd/vim-grammarous and forgot it was LanguageTool based :D

@wahrwolf
Copy link
Contributor Author

Can you give me an example file, so I can try to reproduce the issue?

@oblitum
Copy link
Contributor

oblitum commented Dec 21, 2018

I've opened a markdown file with the contents of example in issue #1516: #1516 (comment)

@oblitum
Copy link
Contributor

oblitum commented Dec 21, 2018

I get 8 errors, but ALE just highlighted the error on last line, all errors are in that line, which is wrong. Clicking on location list errors, it goes to the right column an error is, but doesn't jump to the affected line.

@wahrwolf
Copy link
Contributor Author

Are you sure, that you inserted newline?
With my copy from the text I got no line breaks at all!

@oblitum
Copy link
Contributor

oblitum commented Dec 21, 2018

Yes, I formatted with gqG.

@wahrwolf
Copy link
Contributor Author

That is weird. I can reproduce the issue, but when I add newlines with s/\./.\r/g everything seems normal...

@wahrwolf
Copy link
Contributor Author

This looks like an issues with languagetool. It reports issues in lines that are not even on the screen...

@oblitum
Copy link
Contributor

oblitum commented Dec 21, 2018

> cat foo.md

Paste your own text here and click the 'Check Text' button. Click the colored
phrases for details on potential errors. or use this text too see an few of of
the problems that LanguageTool can detecd. What do you thinks of grammar
checkers?lease not that they are not perfect.Style issues get a blue marker:
It's 5.M. in the afternoon. LanguageTool 3.8 was released on Thursday, 27 June
2017.

> languagetool foo.md

1.) Line 2, column 42, Rule ID: UPPERCASE_SENTENCE_START
Message: This sentence does not start with an uppercase letter
Suggestion: Or
...red phrases for details on potential errors. or use this text too see an few of of the probl...
                                                ^^                                             

2.) Line 2, column 59, Rule ID: TOO_TO[1]
Message: Did you mean 'to see'?
Suggestion: to see
...etails on potential errors. or use this text too see an few of of the problems that LanguageTool ...
                                                ^^^^^^^                                             

3.) Line 2, column 67, Rule ID: EN_A_VS_AN
Message: Use 'a' instead of 'an' if the following word doesn't start with a vowel sound, e.g. 'a sentence', 'a university'
Suggestion: a
...n potential errors. or use this text too see an few of of the problems that LanguageTool can...
                                                ^^                                             

4.) Line 2, column 74, Rule ID: ENGLISH_WORD_REPEAT_RULE
Message: Possible typo: you repeated a word
Suggestion: of
...tial errors. or use this text too see an few of of the problems that LanguageTool can detecd. W...
                                                ^^^^^                                             

5.) Line 3, column 56, Rule ID: DO_VBZ[1]
Message: Consider using the base form 'think'.
Suggestion: think
...ms that LanguageTool can detecd. What do you thinks of grammar checkers?lease not that they are ...
                                                ^^^^^^                                             

6.) Line 3, column 56, Rule ID: NON3PRS_VERB[4]
Message: The pronoun 'you' must be used with a non-third-person form of a verb: 'think'
Suggestion: think
...ms that LanguageTool can detecd. What do you thinks of grammar checkers?lease not that they are ...
                                                ^^^^^^                                             

7.) Line 4, column 46, Rule ID: SENTENCE_WHITESPACE
Message: Add a space between sentences
Suggestion:  Style
...checkers?lease not that they are not perfect.Style issues get a blue marker: It's 5.M. in the a...
                                                ^^^^^                                             

8.) Line 5, column 8, Rule ID: SENTENCE_WHITESPACE
Message: Add a space between sentences
Suggestion:  M
...rfect.Style issues get a blue marker: It's 5.M. in the afternoon. LanguageTool 3.8 was rele...
                                                ^                                             

9.) Line 5, column 62, Rule ID: DATE_WEEKDAY[1]
Message: The date 27 June 2017 is not a Thursday, but a Tuesday.
... afternoon. LanguageTool 3.8 was released on Thursday, 27 June 2017. 
                                                ^^^^^^^^^^^^^^^^^^^^^^  
Time: 1713ms for 7 sentences (4.1 sentences/sec)

@oblitum
Copy link
Contributor

oblitum commented Dec 21, 2018

Exactly, the --line-by-line parameter cause it to output strange lines, without it (see above) seems fine.

@oblitum
Copy link
Contributor

oblitum commented Dec 21, 2018

Everything gets correctly highlighted without the parameter now removed from pull.

@oblitum
Copy link
Contributor

oblitum commented Dec 21, 2018

I wished ALE linters were better at highlighting words... It just highlights first char in most.

@skywind3000
Copy link

skywind3000 commented Dec 21, 2018

Because most compilers or linters only output a position instead of a text range ?

I am using https://github.com/rhysd/vim-grammarous for LanguageTool right now. It can highlight a range of chars.

@wahrwolf
Copy link
Contributor Author

Okay I'll remove the param!

@oblitum
Copy link
Contributor

oblitum commented Dec 21, 2018

It's a bit of extra work, but, the tool does highlight entire pieces of text while this addition just highlights a single char, so, could that be added? If not, I think it's to be born with an issue open on that.

Related: #2125

@oblitum
Copy link
Contributor

oblitum commented Dec 21, 2018

Possibly just counting the amount of ^^^^^^ in the empty line and using it as the number of chars to highlight will do a good job. See result above.

@oblitum
Copy link
Contributor

oblitum commented Dec 21, 2018

Because most compilers or linters only output a position instead of a text range ?

I think vim-grammarous simply cares to process what I requested above. See #2125.

@wahrwolf
Copy link
Contributor Author

alright, should work just fine!

@oblitum
Copy link
Contributor

oblitum commented Dec 21, 2018

I've never seen a commit stream with that many typos, are you fine @wahrwolf?

@wahrwolf
Copy link
Contributor Author

I am currently working on stuff for my university and am little low on sleep ;)

Copy link
Member

@w0rp w0rp left a comment

Choose a reason for hiding this comment

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

Looks pretty good to me. Add some tests for the linter command and executable, and for the new error handler. See :help ale-development.

Merry Christmas!

ale_linters/markdown/languagetool.vim Outdated Show resolved Hide resolved
ale_linters/markdown/languagetool.vim Outdated Show resolved Hide resolved
@oblitum
Copy link
Contributor

oblitum commented Jan 25, 2019

bump.

@skywind3000
Copy link

skywind3000 commented Jan 25, 2019

I have made linters for ALE too, everything worked fine. But it required tests and I encountered some vader (?? If I remember correctly) integration problems on my laptop, so I give up.

the corpse of my PR is still there:
#1571

The hardest part is not writing the linter, but using vader.

@w0rp
Copy link
Member

w0rp commented Jan 27, 2019

See :help ale-development-linter-tests for information on adding the tests required for linter commands and executables.

@wahrwolf
Copy link
Contributor Author

wahrwolf commented Jan 27, 2019 via email

@nbiederbeck
Copy link

let me write the handler for txt and mail too!

Is it possible (probably with more work) to use this with Latex files?

@oblitum
Copy link
Contributor

oblitum commented Feb 2, 2019

I have made linters for ALE too, everything worked fine. But it required tests and I encountered some vader (?? If I remember correctly) integration problems on my laptop, so I give up.

the corpse of my PR is still there:
#1571

The hardest part is not writing the linter, but using vader.

Heh, I just wrote a very simple linter for Neo4j's Cypher, which is working by the way. I wished to contribute but the tests part of the process look quite annoying for someone not familiar with neither Vader nor the ALE specific test setup.

Copy link
Member

@w0rp w0rp left a comment

Choose a reason for hiding this comment

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

Looks pretty good. List the tool as being supported in README.md and in doc/ale.txt.

autoload/ale/handlers/languagetool.vim Outdated Show resolved Hide resolved
@oblitum
Copy link
Contributor

oblitum commented Mar 8, 2019

bump 😴

@skywind3000
Copy link

@wahrwolf , will you make this final move ??

@wahrwolf
Copy link
Contributor Author

wahrwolf commented Mar 8, 2019 via email

@wahrwolf
Copy link
Contributor Author

wahrwolf commented Mar 8, 2019

@w0rp lets try this :D

@w0rp w0rp merged commit 7eae06d into dense-analysis:master Mar 9, 2019
@w0rp
Copy link
Member

w0rp commented Mar 9, 2019

Cheers! 🍻

@wahrwolf wahrwolf deleted the markdown/languagetool branch March 9, 2019 14:56
@oblitum
Copy link
Contributor

oblitum commented Mar 9, 2019

🚨 🚨 🚨

Sadly this broke mail/markdown/text/etc when languagetool is present. On file write I get some java GUIs popping up due to --autoDetect not being recognized as a file, this happens when you run languangetool --autoDetect without a file parameter, this means the pull is not passing it, there's no %s in the command_callback.

@oblitum
Copy link
Contributor

oblitum commented Mar 9, 2019

I think is the only issue, the missing %s.

@ghost
Copy link

ghost commented Mar 9, 2019

Maybe the languagetool_executable should be set to something like this: java -jar /path/to/languagetool-commandline.jar?

@oblitum
Copy link
Contributor

oblitum commented Mar 9, 2019

@fszymanski why? I see no reason for that.

@oblitum
Copy link
Contributor

oblitum commented Mar 9, 2019

It's configurable. I think languagetool as default is expected.

@skywind3000
Copy link

skywind3000 commented Mar 10, 2019

Cannot be used on Windows:

let ale_linters.markdown = ['languagetool']
let g:ale_languagetool_executable = 'd:/software/LanguageTool/LanguageTool-4.4/languagetool.cmd'

while languagetool.cmd is a batch file for launching languagetool-commandline.jar:

@ECHO off
set "INSTALLDIR=%~dp0"
java -jar "%INSTALLDIR%languagetool-commandline.jar" %*

I can see the java process hangs in the background, and ALEInfo returns:

aleerror

The java process will never finish until I kill it manually, any clue for this ??

@oblitum
Copy link
Contributor

oblitum commented Mar 10, 2019

As I mentioned previously, there's a bug even on Linux, the file is not being passed, there's a %s missing in the pull. This bug has nothing to do with the executable path.

diff --git a/autoload/ale/handlers/languagetool.vim b/autoload/ale/handlers/languagetool.vim
index aaad4c9..10e049d 100644
--- a/autoload/ale/handlers/languagetool.vim
+++ b/autoload/ale/handlers/languagetool.vim
@@ -10,7 +10,7 @@ endfunction
 function! ale#handlers#languagetool#GetCommand(buffer) abort
     let l:executable = ale#handlers#languagetool#GetExecutable(a:buffer)

-    return ale#Escape(l:executable) . ' --autoDetect '
+    return ale#Escape(l:executable) . ' --autoDetect %s'
 endfunction

 function! ale#handlers#languagetool#HandleOutput(buffer, lines) abort

@wahrwolf
Copy link
Contributor Author

will this PR ever pass xD

@oblitum you are right though and I have no idea why I removed the %s...

@oblitum
Copy link
Contributor

oblitum commented Mar 11, 2019

Added #2349 to fix this.

@skywind3000
Copy link

@w0rp , please

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.

5 participants