improve emacs support #353

wgrr · 2021-06-10T19:37:31Z

adds support for eglot, lsp-mode and flymake
use --stdin flag in flycheck

still missing update in README documentation, that's a WIP.

strager

Looks good! I enjoyed reading each commit.

Blocking: But tests are failing on CI. =[ Please fix.

Nit: Commit message of commit d5ebeb6 says 'Flamake', but I assume you meant 'Flymake'?

strager · 2021-06-10T21:09:20Z

plugin/emacs/flycheck-quicklintjs.el

+  :error-explainer (lambda (err)
+                     (let ((error-code (flycheck-error-id err))
+                           (url "https://quick-lint-js.com/errors/#%s"))
+                       (and error-code `(url . ,(format url error-code)))))


How would you pronounce this expression aloud? =S (I'm an Elisp newb.)

"error-code and quote to cons url onto string" perhaps? not sure if that helps much, but it's somehow equialent to:

(and error-code (cons url (format url error-code)))

the backtick quoting differs from apostrophe in that it allows some evaluation inside the quoting, here ,(format url error-code) becomes a cons which car is https://quick-lint-js.com/errors/#%s and cdr is https://quick-lint-js.com/errors/#ENNN

strager · 2021-06-10T21:11:50Z

plugin/emacs/eglot-quicklintjs.el

+  :type '(repeat string))
+
+(add-to-list 'eglot-server-programs `(js-mode . (,eglot-quicklintjs-program
+                                                  "--lsp"


Nit: Prefer the full option name in scripts:

Suggested change

"--lsp"

"--lsp-server"

strager · 2021-06-10T21:12:54Z

plugin/emacs/eglot-quicklintjs.el

@@ -0,0 +1,62 @@
+;;; eglot-quicklintjs --- Eglot support for quick-lint-js   -*- lexical-binding: t; -*-


Can this be called eglot-quick-lint-js? (Same with flymake and lsp-mode.) Or are hyphens disallowed?

this is supposed to be shuold be the file name, it's missing a .el

strager · 2021-06-10T21:13:48Z

plugin/emacs/test/quicklintjs-test.el

+  (mapcar (lambda (pkg) (unless (package-installed-p pkg)
+                          (if (> emacs-major-version 24)
+                              (package-install pkg t)
+                            (package-install pkg)))) deps))


Nit: Indentation looks off to me. Shouldn't there be two more spaces before this line to align it with line 15?

I've been indenting with a script that runs emacs commad "indent-region" in the file, it does indent as you're looking, and lisp code inside emacs /lisp directory is also indented like this:

(if (> 0 0) ; cond (foo) ; then (bar) ; else

so i think it's correct according to Emacs standards

strager · 2021-06-10T21:22:03Z

plugin/emacs/lsp-quicklintjs.el

+
+(lsp-register-client
+ (make-lsp-client
+  :new-connection (lsp-stdio-connection   `(,lsp-quicklintjs-program "--lsp"


Nit: Prefer the full option name in scripts:

Suggested change

:new-connection (lsp-stdio-connection `(,lsp-quicklintjs-program "--lsp"

:new-connection (lsp-stdio-connection `(,lsp-quicklintjs-program "--lsp-server"

strager · 2021-06-10T21:23:44Z

plugin/emacs/lsp-quicklintjs.el

+
+(lsp-register-client
+ (make-lsp-client
+  :new-connection (lsp-stdio-connection   `(,lsp-quicklintjs-program "--lsp"


Nit: Collapse interior whitespace?

Suggested change

:new-connection (lsp-stdio-connection `(,lsp-quicklintjs-program "--lsp"

:new-connection (lsp-stdio-connection `(,lsp-quicklintjs-program "--lsp"

good catches, i can't tell the difference in the editor, my font is too small :(

strager · 2021-06-10T21:23:52Z

plugin/emacs/test/quicklintjs-test.el

+  (when (>= emacs-major-version 26)
+    (require 'lsp-quicklintjs)
+    (ert-deftest quicklintjs-is-in-lsp-clients ()
+      (should  (gethash 'quick-lint-js lsp-clients)))))


Nit: Collapse interior whitespace?

Suggested change

(should (gethash 'quick-lint-js lsp-clients)))))

(should (gethash 'quick-lint-js lsp-clients)))))

strager · 2021-06-10T21:30:02Z

plugin/emacs/flymake-quicklintjs.el

@@ -0,0 +1,138 @@
+;;; flymake-quicklintjs.el ---  Flymake support for quick-lint-js -*- lexical-binding: t; -*-


I see why people prefer Flycheck over Flymake... You sure have to do a lot of work yourself.

strager · 2021-06-10T21:34:46Z

plugin/emacs/flymake-quicklintjs.el

+                           (funcall report-fn
+                                    :panic :explanation
+                                    (buffer-substring
+                                     (point-min) (progn (goto-char (point-min))
+                                                        (line-end-position)))))))))


👍

Can you show what a panic looks like in Emacs' UI?

Can you show what a panic looks like in Emacs' UI?

There is no UI for this, a "panic" just tells that flymake that the backend should be disable and logs the :explanation in flymake log buffer, when i first made this i think i was stuck with a crash/restart.

wgrr · 2021-06-11T02:47:14Z

Blocking: But tests are failing on CI. =[ Please fix.

I think flymake have had a rework after version 26 of Emacs, i'll check what's with up with version 24 that's running ni C.I

Nit: Commit message of commit d5ebeb6 says 'Flamake', but I assume you meant 'Flymake'?
Yes it's typo

wgrr · 2021-06-11T13:38:05Z

@strager The issue with CI was that Emacs 24 has a complete different Flymake implementation, there is no easy way to fix this other than skiping, I feel we should upgrade emacs to at least 26 in CI, since it's by far the most supported version (minimum version for Flymake, Eglot and lsp-mode) and the current version for debian-stable.

wgrr · 2021-06-11T13:48:24Z

@strager force pushed to fix a merge conflict, ill be updating the README later tonight

strager · 2021-06-13T18:28:22Z

test/test-lsp-server.cpp

+                            ::simdjson::ondemand::value& uri,
+                            ::simdjson::ondemand::value& version,
+                            byte_buffer& notification_json) {
+    EXPECT_EQ(code, u8"let x = x;");
+    EXPECT_EQ(simdjson_to_jsoncpp(uri), "file:///test.js");
+    EXPECT_EQ(simdjson_to_jsoncpp(version), 10);


Blocking: uri and version changed to string8_view. This is the cause of your build failures:

Suggested change

::simdjson::ondemand::value& uri,

::simdjson::ondemand::value& version,

byte_buffer& notification_json) {

EXPECT_EQ(code, u8"let x = x;");

EXPECT_EQ(simdjson_to_jsoncpp(uri), "file:///test.js");

EXPECT_EQ(simdjson_to_jsoncpp(version), 10);

string8_view uri_json,

string8_view version_json,

byte_buffer& notification_json) {

EXPECT_EQ(code, u8"let x = x;");

EXPECT_EQ(uri_json, u8"\"file:///test.js\"");

EXPECT_EQ(version_json, u8"10");

strager · 2021-06-13T18:32:35Z

Aside from the test failures, this PR looks good to me.

FYI: When I merge, I'm gonna apply each commit (not squash), except I'm going to fold 311a201 into 8f76d86 and 775e956.

Signed-off-by: wagner riffel <[email protected]>

Also while at it, remove tabs accidentally introduced by vim and correct the Flycheck spelling. Signed-off-by: wagner riffel <[email protected]>

This commit also ignores client notifications that would make quicklintjs to crash and accept "js" as a language_id for files, that's what Eglot uses for JavaScript. Signed-off-by: wagner riffel <[email protected]>

Signed-off-by: wagner riffel <[email protected]>

wgrr · 2021-06-13T21:17:20Z

@strager
oops, i didn't noticed the change makes it fail, my bad.

Also, folded 311a201 into 8f76d86 and 775e956

wgrr force-pushed the emacs branch from 8e55850 to d5ebeb6 Compare June 10, 2021 19:57

strager approved these changes Jun 10, 2021

View reviewed changes

strager linked an issue Jun 11, 2021 that may be closed by this pull request

emacs plugin development track #275

Open

11 tasks

wgrr force-pushed the emacs branch from d5ebeb6 to 1892780 Compare June 11, 2021 13:15

wgrr force-pushed the emacs branch from 1892780 to 4b6e430 Compare June 11, 2021 13:47

wgrr force-pushed the emacs branch from 4b6e430 to 311a201 Compare June 11, 2021 17:22

wgrr requested a review from strager June 13, 2021 14:25

strager approved these changes Jun 13, 2021

View reviewed changes

wgrr added 7 commits June 13, 2021 22:24

plugin/emacs: use --stdin in flycheck

5b3e588

Signed-off-by: wagner riffel <[email protected]>

plugin/emacs: support Flycheck error explainer

05aff9d

Also while at it, remove tabs accidentally introduced by vim and correct the Flycheck spelling. Signed-off-by: wagner riffel <[email protected]>

plugin/emacs: add Eglot support

54bc9eb

This commit also ignores client notifications that would make quicklintjs to crash and accept "js" as a language_id for files, that's what Eglot uses for JavaScript. Signed-off-by: wagner riffel <[email protected]>

plugin/emacs: add LSP Mode support

88874b3

Signed-off-by: wagner riffel <[email protected]>

plugin/emacs: add Flymake backend support

fc0b98b

Signed-off-by: wagner riffel <[email protected]>

plugin/emacs: skip test when Emacs version is lower than 26

15020fd

Signed-off-by: wagner riffel <[email protected]>

plugin/emacs: add installation steps in README for new integrations

d7e006a

Signed-off-by: wagner riffel <[email protected]>

wgrr force-pushed the emacs branch from bb95257 to d7e006a Compare June 13, 2021 21:14

strager merged commit d7e006a into quick-lint:master Jun 13, 2021

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

improve emacs support #353

improve emacs support #353

wgrr commented Jun 10, 2021

strager left a comment

strager Jun 10, 2021

wgrr Jun 11, 2021

strager Jun 10, 2021

strager Jun 10, 2021

wgrr Jun 11, 2021

strager Jun 10, 2021

wgrr Jun 11, 2021

strager Jun 10, 2021

strager Jun 10, 2021

wgrr Jun 11, 2021 •

edited

Loading

strager Jun 10, 2021

strager Jun 10, 2021

strager Jun 10, 2021

wgrr Jun 11, 2021

wgrr commented Jun 11, 2021

wgrr commented Jun 11, 2021

wgrr commented Jun 11, 2021

strager Jun 13, 2021

strager commented Jun 13, 2021

wgrr commented Jun 13, 2021

		@@ -0,0 +1,62 @@
		;;; eglot-quicklintjs --- Eglot support for quick-lint-js -- lexical-binding: t; --

	:new-connection (lsp-stdio-connection `(,lsp-quicklintjs-program "--lsp"
	:new-connection (lsp-stdio-connection `(,lsp-quicklintjs-program "--lsp-server"

	(should (gethash 'quick-lint-js lsp-clients)))))
	(should (gethash 'quick-lint-js lsp-clients)))))

		@@ -0,0 +1,138 @@
		;;; flymake-quicklintjs.el --- Flymake support for quick-lint-js -- lexical-binding: t; --

improve emacs support #353

improve emacs support #353

Conversation

wgrr commented Jun 10, 2021

strager left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wgrr Jun 11, 2021 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

wgrr commented Jun 11, 2021

wgrr commented Jun 11, 2021

wgrr commented Jun 11, 2021

Choose a reason for hiding this comment

strager commented Jun 13, 2021

wgrr commented Jun 13, 2021

wgrr Jun 11, 2021 •

edited

Loading