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

languagetools: stop auto-appending --autoDetect #2616

Merged

Conversation

pappasam
Copy link
Contributor

  • I've updated the relevant tests and run them; they pass

This pull request fixes an issue where Ale prevents the user from overriding a broken setting in languagetools (--autoDetect). --autoDetect conflicts with a local
command line usage of languagetools with the ngram data installed
locally. In my case, I've downloaded "en" ngrams with support for the English language.

If you want to retain current behavior, you can change the default value for the languagetools command. That said, since it appears to be somewhat broken, it's probably better to just default to languagetools with no options (as this PR currently implements).

Design-wise, auto-appending anything to a command is a problem because it prevents
users from overriding the appended configuration. There was no way to
remove --autoDetect from the executed command without this commit /
updating the plugin source code. If a user wants autoDetect they are free to place it in their bash script (or other path-included script).

Example command (in a bashscript in my path called languagetools):

#!/bin/bash
java -jar ~/java/LanguageTool-4.5/languagetool-commandline.jar \
  --languagemodel $HOME/Data/ngrams \
  ${@}

By placing "--autoDetect" before my filename, languagetools would only
return linting messages unassociated with ngrams (which are necessary
for checking "their" versus "there" errors). Once I manually removed
this line, everything works correctly.

Example with ngrams and NO autoDetect

image

notice the second suggestion for "their" versus "there"

Example with ngrams and autoDetect

image

notice there is only 1 suggestion

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.

I think the best way to handle this is to add a new _options variable where the default contains this option. That way nothing will change for people who currently rely on this option, and anyone who doesn't want the option can remove it in their vimrc file. Have a look at other linters for examples and examples of tests for options.

@w0rp w0rp added this to In Review in Old Working List via automation Jul 14, 2019
@pappasam
Copy link
Contributor Author

pappasam commented Jul 17, 2019

@w0rp the _options variable has been created. I've updated the tests and the docs accordingly. Please let me know if there's anything else required.

@abers
Copy link

abers commented Jul 18, 2019

I don't use ngrams but experiencing issue with --autoDetect resulting in the following output:

[main] DEBUG com.optimaize.langdetect.LanguageDetectorImpl - ==> [DetectedLanguage[en:0.9999988934696109 ]] 18:56:09.429 [main] DEBUG com.optimaize.langdetect.LanguageDetectorImpl - ==> [DetectedLanguage[en:0.9999990562851635 ]] 18:56:09.429 [main] DEBUG com.optimaize.langdetect.LanguageDetectorImpl - ==> [DetectedLanguage[en:0.9999984259788106 ]] 18:56:09.429 [main] DEBUG com.optimaize.langdetect.LanguageDetectorImpl - ==> [DetectedLanguage[en:0.9999962700310778 ]]

Changing '--autoDetect' to -'-language en-GB' in the handler fixes the issue.

@Konfekt
Copy link

Konfekt commented Jul 21, 2019

It seems that LanguageTool checks for ngram errors or alike by default when --language is passed, but not --autoDetect as there are less error messages using the latter.

In the meanwhile, you could work around this issue by my simple plug-in (together with, say, vim-dispatch to run the LanguageTool grammar check in the background) where all parameters passed can be entirely overridden.

@pappasam
Copy link
Contributor Author

I don't use ngrams but experiencing issue with --autoDetect resulting in the following output:

[main] DEBUG com.optimaize.langdetect.LanguageDetectorImpl - ==> [DetectedLanguage[en:0.9999988934696109 ]] 18:56:09.429 [main] DEBUG com.optimaize.langdetect.LanguageDetectorImpl - ==> [DetectedLanguage[en:0.9999990562851635 ]] 18:56:09.429 [main] DEBUG com.optimaize.langdetect.LanguageDetectorImpl - ==> [DetectedLanguage[en:0.9999984259788106 ]] 18:56:09.429 [main] DEBUG com.optimaize.langdetect.LanguageDetectorImpl - ==> [DetectedLanguage[en:0.9999962700310778 ]]

Changing '--autoDetect' to -'-language en-GB' in the handler fixes the issue.

@w0rp it seems like the comments in this thread are pointing out that --autoDetect may be a bad default. That said, this PR makes it easily override-able. If there is nothing else to do, would we be able to merge this soon? I'd like to stop using my personal fork for daily work, if possible.

@w0rp w0rp added this to In Review in On the Radar Aug 17, 2019
Auto-appending anything to the command is a problem because it prevents
users from overriding the appended configuration. There was no way to
remove --autoDetect from the executed command without this commit /
updating the plugin source code.

In this particular instance, --autoDetect conflicts with a local
command line usage of languagetools with the ngram data installed
locally.

Example command (in a bashscript in my path called languagetools):

    #!/bin/bash
    java -jar ~/java/LanguageTool-4.5/languagetool-commandline.jar \
      --languagemodel $HOME/Data/ngrams \
      ${@}

By placing "--autoDetect" before my filename, languagetools would only
return linting messages unassociated with ngrams (which are necessary
for checking "their" versus "there" errors). Once I manually removed
this line, everything works correctly.
Enables overriding the options passed to the languagetools binary. This
enables users to override "--autoDetect" while preserving the old
behavior.
@pappasam pappasam force-pushed the languagetool_autodetect_breaks_ngrams branch from 1c85e2e to 68da041 Compare August 30, 2019 19:09
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.

This looks good to me. Sorry for the delay. I'm slowly catching up on issues and pull requests I haven't seen yet.

@w0rp w0rp merged commit 05ba522 into dense-analysis:master Sep 12, 2019
On the Radar automation moved this from In Review to Done Sep 12, 2019
timlag1305 pushed a commit to timlag1305/ale that referenced this pull request Nov 5, 2019
Options are now configurable for languagetools, and `--autoDetect` can be removed by changing the options.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
Development

Successfully merging this pull request may close these issues.

None yet

4 participants