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

use flymake-eslint-executable to access eslint indirectly (#26) #34

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

aecepoglu
Copy link

This is a NON-backward-compatible work that replaces flymake-eslint-executable-name and flymake-eslint-executable-args with a single list-type argument: flymake-eslint-executable .

This way we are able to:

  1. call eslint through npx (Allow running ESLint using npx #26)
  2. call eslint in node_modules/.bin (Prefer locally installed eslint when looking for the executable #29)
  3. have one fewer variable to worry about

I've also replaced the existence check with a --version call. I have not really considered its implications though.

Also listed some example values for flymake-eslint-executable in its docs.

Please edit the work as you please so we can get this merged and the issue sorted. Allowing flymake-eslint-executable-name to be a list could be a backward-compatible version of this PR.

@DamienCassou
Copy link
Contributor

There are some conflicts on this PR.

@aecepoglu
Copy link
Author

Sorted.

(defcustom flymake-eslint-executable '("eslint")
"Arguments to execute `eslint'
Example values:
'(\"eslint\") ; to use an eslint available in exec-ptah
Copy link
Author

Choose a reason for hiding this comment

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

typo

'(\"npx\" \"eslint\") ; to use eslint through npx. Allows access to project-local eslint
'(\"npm\" \"exec\" \"--\" \"eslint\") ; equivalent to \"npx\"

You can also send additional arguments to eslint. For example to set eslint's --max-warnings to 13:
Copy link
Author

Choose a reason for hiding this comment

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

"also" redundant.

@aguynamedben
Copy link

aguynamedben commented Jun 27, 2024

Update:

I think my pain was due to Eglot + flymake-eslint. This is working now, inspired by schmir/.emacs.d@f72fcee. Eglot is overly-aggressive and takes over flymake backends, so we have to tell it to not completely take over flymake, then turn on Elgot's flymake backend and flymake-eslint, see joaotavora/eglot#268

;; The Emacs Client for LSP servers
(use-package eglot
  ;; https://github.com/schmir/.emacs.d/commit/f72fcee017aaba3319d1d6e43289210457a91234
  :config
  (defun my/setup-eglot-flymake-backend ()
    "Enable eglot's flymake backend manually."
    (add-hook 'flymake-diagnostic-functions #'eglot-flymake-backend nil t))

  (with-eval-after-load 'eglot
    (setopt eglot-autoshutdown t)
    ;; let me manage flymake on my own
    (add-to-list 'eglot-stay-out-of 'flymake)))

;; Flymake is an Emacs minor mode for on-the-fly syntax checking.
(use-package flymake
  :bind
  (("M-n" . flymake-goto-next-error)
   ("M-p" . flymake-goto-prev-error)))

(use-package flymake-eslint
  :straight (flymake-eslint :type git :host github :repo "aecepoglu/flymake-eslint" :commit "0dec52c2f059b17250e32cb3548c8ed4ac198951")
  :custom
  (flymake-eslint-executable '("npx" "eslint")))

(use-package typescript-ts-mode
  :ensure nil
  :mode
  ("\\.ts\\'" . typescript-ts-mode)
  ("\\.tsx\\'" . tsx-ts-mode)
  :hook
  (typescript-ts-mode . eglot-ensure)
  (typescript-ts-mode . flymake-mode)
  (typescript-ts-mode . my/setup-eglot-flymake-backend)
  (typescript-ts-mode . flymake-eslint-enable)
  (typescript-ts-mode . prettier-js-mode)
  (typescript-ts-mode . copilot-mode))

Original Question:
@aecepoglu Thank you for contributing this change, as I've spent hours trying to get flymake-eslint to work as I wish.

Can you give an example of using this with use-package?

I'm trying this:

(use-package flymake-eslint
  :straight (flymake-eslint :type git :host github :repo "aecepoglu/flymake-eslint" :commit "0dec52c2f059b17250e32cb3548c8ed4ac198951")
  :custom
  (flymake-eslint-executable '("npx" "eslint")))

(use-package typescript-ts-mode
  :ensure nil
  :mode
  ("\\.ts\\'" . typescript-ts-mode)
  ("\\.tsx\\'" . tsx-ts-mode)
  :hook
  (typescript-ts-mode . eglot-ensure)
  (typescript-ts-mode . flymake-eslint-enable)
  (typescript-ts-mode . prettier-js-mode)
  (typescript-ts-mode . copilot-mode)
  (tsx-ts-mode . eglot-ensure)
  (tsx-ts-mode . flymake-eslint-enable)
  (tsx-ts-mode . prettier-js-mode)
  (tsx-ts-mode . copilot-mode))

And when I first open a TypeScript file I get

error in process sentinel: Can’t find state for flymake-eslint--checker in ‘flymake--state’ [2 times]

in the echo area and Message buffer, but in the same buffer if I run M-x flymake-eslint-enable again, it starts working.

@aguynamedben
Copy link

This branch is working well for me and makes flymake-eslint a lot easier to configure. 👏

:type 'string
:group 'flymake-eslint)

(defcustom flymake-eslint-executable-args nil
Copy link
Owner

Choose a reason for hiding this comment

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

completely removing this symbol will cause this package to break for any user who has assigned a value to it.

I suggest continuing to support it for now but apply the define-obsolete-variable-alias macro to tell users they should prefer flymake-eslint-executable instead.

Copy link
Author

Choose a reason for hiding this comment

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

That is true. I welcome you to edit the code as you see fit to respect the older symbols.

@aecepoglu
Copy link
Author

@aguynamedben I enable/disable flymake manually so I haven't noticed anything. Your use-package setup seems right though.

I'll keep an eye out for errors I see.

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.

4 participants