wip: fix #396, #395 and #398 #409
There are no files selected for viewing
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
| @@ -0,0 +1,81 @@ | ||||||
| # Copyright (C) 2020 Matthew "strager" Glazar | ||||||
| # See end of file for extended copyright information. | ||||||
|
|
||||||
| if (QUICK_LINT_JS_EMACS) | ||||||
| set(QUICK_LINT_JS_EMACS "${QUICK_LINT_JS_EMACS}") | ||||||
| else () | ||||||
| find_program(QUICK_LINT_JS_EMACS "emacs") | ||||||
| endif () | ||||||
|
|
||||||
| if (NOT QUICK_LINT_JS_EMACS) | ||||||
| message(WARNING "Emacs not found. Skipping... ") | ||||||
|
||||||
| message(WARNING "Emacs not found. Skipping... ") |
Outdated
There was a problem hiding this comment.
Nit: Match format of similar messages in CMake:
| message(STATUS "Found Emacs: (${QUICK_LINT_JS_EMACS}) suitable version ${EMACS_VERSION} minimum required is 24.5") | |
| message(STATUS "Found Emacs: ${QUICK_LINT_JS_EMACS} (found suitable version \"${EMACS_VERSION}\", minimum required is \"24.5\"") |
Outdated
There was a problem hiding this comment.
Nit: I'd put this macro in plugin/emacs/CMakeLists.txt instead of here. This macro is tightly coupled to quicklintjs-pkg.el, so I think the two belong next to each other.
Outdated
There was a problem hiding this comment.
Why add CMAKE_CACHEFILE_DIR? What files are in the build dir that we need to expose to Emacs?
There was a problem hiding this comment.
that's the output directory for the package, otherwise Emacs would make it somewhere on its defaults, (probably ~/.emacs.d/elpa), i used the CMAKE_CACHEFILE_DIR because i assume cmake calls ${QUICK_LINT_JS_EMACS} with current directory being the same from it was invoked from, so the root of build directory would have been filled with emacs package folders instead of ${BUILD_DIR}/plugins/emacs. I cloud've made this from emacs by appending plugins/emacs to cwd, or you have something else in mind?
There was a problem hiding this comment.
that's the output directory for the package
Oh. It looked like an input directory to me.
${CMAKE_CURRENT_BINARY_DIR} might be better.
I cloud've made this from emacs by appending
plugins/emacsto cwd, or you have something else in mind?
I think specifying cwd explicitly is what we should do:
add_custom_command(
OUTPUT ${_OUTPUT}
COMMAND
${QUICK_LINT_JS_EMACS}
-Q -batch
-l package --eval "(package-initialize)"
--eval "(add-to-list 'package-directory-list .)"
...
WORKING_DIRECTORY "${CMAKE_CURRENT_BINARY_DIR}"
...
)
Outdated
There was a problem hiding this comment.
Minor: This command is pretty noisy during building. Can we silence the output unless there is an error or warning?
[23/226] Generating quicklintjs-0.0.1
Generating autoloads for quicklintjs.el...
Generating autoloads for quicklintjs.el...done
Wrote /home/strager/Projects/quicklint-js/build-clang-stage3/plugin/emacs/quicklintjs-0.0.1/quicklintjs-autoloads.el
Wrote /home/strager/Projects/quicklint-js/build-clang-stage3/plugin/emacs/quicklintjs-0.0.1/quicklintjs-autoloads.el
Checking /home/strager/Projects/quicklint-js/build-clang-stage3/plugin/emacs/quicklintjs-0.0.1...
Compiling /home/strager/Projects/quicklint-js/build-clang-stage3/plugin/emacs/quicklintjs-0.0.1/quicklintjs-autoloads.el...
Compiling /home/strager/Projects/quicklint-js/build-clang-stage3/plugin/emacs/quicklintjs-0.0.1/quicklintjs-pkg.el...
Compiling /home/strager/Projects/quicklint-js/build-clang-stage3/plugin/emacs/quicklintjs-0.0.1/quicklintjs.el...
Done (Total of 1 file compiled, 2 skipped)
[33/226] Generating flycheck-quicklintjs-0.0.1
Generating autoloads for flycheck-quicklintjs.el...
Generating autoloads for flycheck-quicklintjs.el...done
Wrote /home/strager/Projects/quicklint-js/build-clang-stage3/plugin/emacs/flycheck-quicklintjs-0.0.1/flycheck-quicklintjs-autoloads.el
Wrote /home/strager/Projects/quicklint-js/build-clang-stage3/plugin/emacs/flycheck-quicklintjs-0.0.1/flycheck-quicklintjs-autoloads.el
Unable to activate package ‘flycheck-quicklintjs’.
Required package ‘quicklintjs-0.0.1’ is unavailable
[34/226] Generating eglot-quicklintjs-0.0.1
Generating autoloads for eglot-quicklintjs.el...
Generating autoloads for eglot-quicklintjs.el...done
Wrote /home/strager/Projects/quicklint-js/build-clang-stage3/plugin/emacs/eglot-quicklintjs-0.0.1/eglot-quicklintjs-autoloads.el
Wrote /home/strager/Projects/quicklint-js/build-clang-stage3/plugin/emacs/eglot-quicklintjs-0.0.1/eglot-quicklintjs-autoloads.el
Unable to activate package ‘eglot-quicklintjs’.
Required package ‘quicklintjs-0.0.1’ is unavailable
[35/226] Generating flymake-quicklintjs-0.0.1
Generating autoloads for flymake-quicklintjs.el...
Generating autoloads for flymake-quicklintjs.el...done
Wrote /home/strager/Projects/quicklint-js/build-clang-stage3/plugin/emacs/flymake-quicklintjs-0.0.1/flymake-quicklintjs-autoloads.el
Wrote /home/strager/Projects/quicklint-js/build-clang-stage3/plugin/emacs/flymake-quicklintjs-0.0.1/flymake-quicklintjs-autoloads.el
Unable to activate package ‘flymake-quicklintjs’.
Required package ‘quicklintjs-0.0.1’ is unavailable
[36/226] Generating lsp-quicklintjs-0.0.1
Generating autoloads for lsp-quicklintjs.el...
Generating autoloads for lsp-quicklintjs.el...done
Wrote /home/strager/Projects/quicklint-js/build-clang-stage3/plugin/emacs/lsp-quicklintjs-0.0.1/lsp-quicklintjs-autoloads.el
Wrote /home/strager/Projects/quicklint-js/build-clang-stage3/plugin/emacs/lsp-quicklintjs-0.0.1/lsp-quicklintjs-autoloads.el
Unable to activate package ‘lsp-quicklintjs’.
Required package ‘quicklintjs-0.0.1’ is unavailable
| Original file line number | Diff line number | Diff line change | ||||
|---|---|---|---|---|---|---|
|
|
@@ -4,13 +4,54 @@ | |||||
| cmake_minimum_required(VERSION 3.10) | ||||||
| include(GNUInstallDirs) | ||||||
|
|
||||||
| find_package(QuickLintJsEmacs) | ||||||
|
||||||
| find_package(QuickLintJsEmacs) | |
| find_package(QuickLintJSEmacs) |
There was a problem hiding this comment.
Why do we need five separate packages? Could we make just one package? Seems simpler to make one package IMO.
There was a problem hiding this comment.
Could we make just one package?
Yes, to use what emacs calls "multi-file" packages, it would require tar available while building the packages tho, i didn't use it here because i imagine that's unlikely to tar be available on a Windows machine, what you think? i'm fine with both ways.
There was a problem hiding this comment.
it would require
taravailable while building the packages tho, i didn't use it here because i imagine that's unlikely to tar be available on a Windows machine, what you think?
CMake provides a tool to create tar archives: https://cmake.org/cmake/help/v3.21/manual/cmake.1.html#run-a-command-line-tool
$ cmake -E tar czf src.tar.gz src
$ file src.tar.gz
src.tar.gz: gzip compressed data, last modified: Fri Aug 13 18:15:35 2021, from Unix, original size 1392640
There was a problem hiding this comment.
@strager i made the change to use a tar archive as a single package, besides that it feel clunky, it's discouraged by package archives, do you think the last "problem" in this list applies here? i do think. https://github.com/melpa/melpa/blob/5d49574/CONTRIBUTING.org#fixing-typical-problems
There was a problem hiding this comment.
it's discouraged by package archives
Having multiple .el files in a package is discouraged? O_o I'm unsure if that's what the documentation actually says.
do you think the last "problem" in this list applies here? i do think. https://github.com/melpa/melpa/blob/5d49574/CONTRIBUTING.org#fixing-typical-problems
It says:
MELPA only looks at the main .el file (or -pkg.el file, if provided, though we discourage that).
What does it mean to "look at" an .el file?
There was a problem hiding this comment.
What does it mean to "look at" an .el file?
In short, an .el file can be package on its own, its format something along the lines:
;;; <name>.el ---
;;; <headers>
<code>
;;; <name>.el ends here
so when it says "look at" an .el file, is an .el file which is in this package format, when a package has multiple .el files, this format is not used, instead there should be a tar archive that contains all .el files along with a magic <name>-pkg.el, which must call (define-package ...args).
There was a problem hiding this comment.
[a tar archive is] discouraged by package archives, do you think the last "problem" in this list applies here? i do think. https://github.com/melpa/melpa/blob/5d49574/CONTRIBUTING.org#fixing-typical-problems
I think we should do one of two things:
- merge all of our .el files into one .el file
- create a tar, even though MELPA docs say "we discourage that"
I'm happy with the tar if it's already implemented and if it makes developing the plugin easier for us.
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,5 +1,28 @@ | ||
| ;;; eglot-quicklintjs.el --- Eglot support for quick-lint-js -*- lexical-binding: t; -*- | ||
|
|
||
| ;; Copyright (C) 2020 Matthew "strager" Glazar | ||
|
|
||
| ;; Version: 0.0.1 | ||
| ;; Author: Wagner Riffel <[email protected]> | ||
| ;; URL: https://quick-lint-js.com | ||
| ;; Keywords: languages, tools | ||
| ;; Package-Requires: ((quicklintjs "0.0.1") (eglot "1.7") (emacs "26.1")) | ||
|
|
||
| ;; This file is part of quick-lint-js. | ||
| ;; | ||
| ;; quick-lint-js is free software: you can redistribute it and/or modify | ||
| ;; it under the terms of the GNU General Public License as published by | ||
| ;; the Free Software Foundation, either version 3 of the License, or | ||
| ;; (at your option) any later version. | ||
| ;; | ||
| ;; quick-lint-js is distributed in the hope that it will be useful, | ||
| ;; but WITHOUT ANY WARRANTY; without even the implied warranty of | ||
| ;; MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the | ||
| ;; GNU General Public License for more details. | ||
| ;; | ||
| ;; You should have received a copy of the GNU General Public License | ||
| ;; along with quick-lint-js. If not, see <https://www.gnu.org/licenses/>. | ||
|
|
||
| ;;; Commentary: | ||
|
|
||
| ;; Eglot support for quick-lint-js. | ||
|
|
@@ -19,44 +42,13 @@ | |
| ;;; Code: | ||
|
|
||
| (require 'eglot) | ||
| (require 'quicklintjs) | ||
|
|
||
| ;;;###autoload | ||
| (with-eval-after-load 'eglot | ||
| (add-to-list 'eglot-server-programs `(js-mode . ,(quicklintjs-find-program | ||
| "--lsp-server")))) | ||
|
|
||
| (provide 'eglot-quicklintjs) | ||
|
|
||
| ;;; eglot-quicklintjs.el ends here | ||
| Original file line number | Diff line number | Diff line change |
|---|---|---|
| @@ -1,4 +1,27 @@ | ||
| ;;; flycheck-quicklintjs.el --- quick-lint-js Flycheck support -*- lexical-binding: t; -*- | ||
|
|
||
| ;; Copyright (C) 2020 Matthew "strager" Glazar | ||
|
|
||
| ;; Version: 0.0.1 | ||
| ;; Author: Wagner Riffel <[email protected]> | ||
| ;; URL: https://quick-lint-js.com | ||
| ;; Keywords: languages, tools | ||
| ;; Package-Requires: ((quicklintjs "0.0.1") (flycheck "32-cvs") (emacs "24.5")) | ||
|
|
||
| ;; This file is part of quick-lint-js. | ||
| ;; | ||
| ;; quick-lint-js is free software: you can redistribute it and/or modify | ||
| ;; it under the terms of the GNU General Public License as published by | ||
| ;; the Free Software Foundation, either version 3 of the License, or | ||
| ;; (at your option) any later version. | ||
| ;; | ||
| ;; quick-lint-js is distributed in the hope that it will be useful, | ||
| ;; but WITHOUT ANY WARRANTY; without even the implied warranty of | ||
| ;; MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. See the | ||
| ;; GNU General Public License for more details. | ||
| ;; | ||
| ;; You should have received a copy of the GNU General Public License | ||
| ;; along with quick-lint-js. If not, see <https://www.gnu.org/licenses/>. | ||
|
|
||
| ;;; Commentary: | ||
|
|
||
|
|
@@ -35,19 +58,10 @@ | |
| ;;; Code: | ||
|
|
||
| (require 'flycheck) | ||
| (require 'quicklintjs) | ||
|
|
||
| (defun flycheck-quicklintjs-parse-errors (output checker buffer) | ||
| "Parse CHECKER quick-lint-js alist output format from OUTPUT." | ||
| (mapcar (lambda (l) | ||
| (let ((region (nth 0 l)) | ||
| (sev (nth 1 l)) | ||
|
|
@@ -60,48 +74,31 @@ | |
| :id code | ||
| :buffer buffer | ||
| :checker checker | ||
| :end-pos (cdr region)))) | ||
| (car (read-from-string output)))) | ||
|
|
||
| ;;;###autoload | ||
| (flycheck-define-command-checker 'javascript-quicklintjs | ||
| "quick-lint-js finds bugs in JavaScript programs. | ||
|
|
||
| https://quick-lint-js.com" | ||
|
Comment on lines
-71
to
-74
Collaborator
There was a problem hiding this comment. Did you intentionally rewrite this code in this commit? This change seems unrelated to unifying settings. (The change looks fine, though.)
Contributor
Author
There was a problem hiding this comment.
No, I think I merged the wrong piece on a conflict, i'll revert |
||
| :command (quicklintjs-find-program | ||
| "--output-format=emacs-lisp" | ||
| (let ((file (buffer-file-name))) | ||
| (when file (concat "--path-for-config-search=" file))) | ||
| "--stdin") | ||
| :standard-input 't | ||
| :error-parser 'flycheck-quicklintjs-parse-errors | ||
| :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))))) | ||
| :modes 'js-mode) | ||
|
|
||
| ;;;###autoload | ||
| (with-eval-after-load 'flycheck | ||
| (add-to-list 'flycheck-checkers 'javascript-quicklintjs t)) | ||
|
|
||
| (provide 'flycheck-quicklintjs) | ||
|
|
||
| ;;; flycheck-quicklintjs.el ends here | ||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
find_programalready checks if the program was found. No need to check yourself: