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

[WiP] Added a prog mode to flycheck-languagetool #25

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

mattiasdrp
Copy link

@mattiasdrp mattiasdrp commented Nov 24, 2022

This is done by filtering out the errors that don't point to a place that has not a comment, a string or a doc face. The checker is added as the next checker of the main flycheck checker for the current mode

I still have some issues as to when the function flycheck-languagetool-flycheck-enable has to be used

In my current setup what I did is:

  (use-package flycheck-languagetool
    :load-path "lisp/flycheck-languagetool/"
    ;; :custom ((flycheck-languagetool-active-modes
    ;;           '(text-mode latex-mode org-mode markdown-mode message-mode prog-mode)))
    :hook ((text-mode . flycheck-languagetool-setup)
           (lsp-mode . (lambda () (lsp-diagnostics-mode 1)
                         (require 'flycheck-languagetool)
                         (flycheck-languagetool-flycheck-enable))))
    ;; :ensure-system-package
    ;;   ("LanguageTool-5.9-stable/languagetool-commandline.jar" . "curl -L https://raw.githubusercontent.com/languagetool-org/languagetool/master/install.sh | sudo bash -a")
    :init
    (setq flycheck-languagetool-server-jar (expand-file-name "~/.emacs.d/LanguageTool-5.9-stable/languagetool-server.jar"))
    )

But this is not really good.

The issue is that if languagetool is added as a checker before the main-mode finished loading, it's not added as a next checker for the main one and the function needs to be called again.

Fixes #22

If this seems good for you, I think an improvement would be to define a predicate function

This would allow to define different predicates for different modes (prog-mode don't have the same predicates as org-mode or markdown-mode for example)

@mattiasdrp mattiasdrp force-pushed the prog-mode branch 2 times, most recently from 0956ef3 to 95a0ae3 Compare November 25, 2022 09:56
(flycheck-languagetool-flycheck-add-mode major-mode)
(add-to-list 'flycheck-checkers 'languagetool)
(setq flycheck-languagetool--text-mode nil)
(flycheck-add-next-checker (flycheck-get-checker-for-buffer) 'languagetool))
Copy link
Member

Choose a reason for hiding this comment

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

Few comments:

  1. Maybe we should get rid of the (require 'flycheck-mode) here since we already have it at the very top.
  2. I don't think enabling flycheck-mode and such a setup is a good idea, we usually let the user decide weather the flycheck-mode is enabled or not. Of course, we can still expose a function for that but we need to clarify so the user don't get confuse between flycheck-languagetool-flycheck-enable and flycheck-languagetool-setup. (Might need to update the README for this)
  3. Should the variable flycheck-languagetool--text-mode be defvar-local? Anyway, I think it's better if we reuse flycheck-languagetool-setup for prog-mode as well. For example,
(defcustom flycheck-languagetool-prog-modes
  '(actionscript-mode haxe-mode nxml-mode yaml-mode)
  "List of extra `prog-mode'.")

(defun flycheck-languagetool-prog-mode-p ()
  "Return non-nil if current buffer is programming mode."
  (or (derived-mode-p 'prog-mode)
      (memq major-mode flycheck-languagetool-prog-modes)))

then

(defun flycheck-languagetool-setup ()
  "Setup flycheck-package."
  (interactive)
  (when (flycheck-languagetool-prog-mode-p)
    (flycheck-languagetool-flycheck-add-mode major-mode)
    (flycheck-add-next-checker (flycheck-get-checker-for-buffer) 'languagetool))
  ;; do it whenever this is programming mode or not
  (add-to-list 'flycheck-checkers 'languagetool))

;;;###autoload
(defun flycheck-languagetool-flycheck-enable ()
  "Enable flycheck languagetool integration for the current buffer.
This adds languagetool as the next checker of the main checker
of the current buffer"
  (interactive)
  (flycheck-mode 1)
  (flycheck-stop)
  (setq flycheck-languagetool--text-mode nil)  ; still not quite sure what this does
  (flycheck-languagetool-setup))  ; reuse it

This way, user wouldn't need to do extra configuration for prog-mode support. BTW, above code is an example, so please change it to make it work! ;)

Copy link
Author

Choose a reason for hiding this comment

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

Thanks for the review! This is clearly a work in progress, I needed to do something minimal that worked to start the process of iterating over it. I'll look into your suggestions :-)

This is done by filtering out the errors that don't point to a place that has not
a comment, a string or a doc face. The checker is added as the next checker of
the main flycheck checker for the current mode

I still have some issues as to when the function `flycheck-languagetool-flycheck-enable` has to be used

In my current setup what I did is:

```elisp
  (use-package flycheck-languagetool
    :load-path "lisp/flycheck-languagetool/"
    ;; :custom ((flycheck-languagetool-active-modes
    ;;           '(text-mode latex-mode org-mode markdown-mode message-mode prog-mode)))
    :hook ((text-mode . flycheck-languagetool-setup)
           (lsp-mode . (lambda () (lsp-diagnostics-mode 1)
                         (require 'flycheck-languagetool)
                         (flycheck-languagetool-flycheck-enable))))
    ;; :ensure-system-package
    ;;   ("LanguageTool-5.9-stable/languagetool-commandline.jar" . "curl -L https://raw.githubusercontent.com/languagetool-org/languagetool/master/install.sh | sudo bash -a")
    :init
    (setq flycheck-languagetool-server-jar (expand-file-name "~/.emacs.d/LanguageTool-5.9-stable/languagetool-server.jar"))
    )
```

But this is not really good.

The issue is that if languagetool is added as a checker before the main-mode
finished loading, it's not added as a next checker for the main one and the
function needs to be called again.
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.

Grammar checking in source code comments
2 participants