Flymake backend based on "renpy lint". #13
Conversation
|
I've tried it out on Windows and it is mostly working for me. Here is how I configured it: (use-package renpy
:defer t
:config
(setq renpy-flymake-command '("D:/renpy-8.3.7-sdk/renpy.exe"))
;; It doesn't make sense to run the lint process on buffer changes.
(add-hook 'renpy-mode-hook
(lambda ()
(setq-local flymake-no-changes-timeout nil))))Ocassionally (I think I've seen it twice in about 50 lints) it seems as though the flymake backend loses track of the process (the mode-line suggests the process is running when it has exited), and it does seem to get confused by old source files and bytecode, but I think that is likely down to the nature of the lint process rather than anything that can be controlled through the flymake backend. I will re-test on Linux the next time I am using it. It works with Was there anything specific that you wanted me to test? I haven't looked too closely at the code, other than what was relevant to configuring it, but here is the feedback I have so far:
(defcustom renpy-program "renpy"
"Specifies the name of the Ren'Py executable."
:type 'string
:group 'renpy)When I researched it a while ago I concluded that the
(add-hook 'flymake-diagnostic-functions #'renpy--flymake-backend nil t)
(setq-local flymake-no-changes-timeout nil)Normally I wouldn't want to set a customization variable, but in this case it feels the best option to stop the lint process being continually (re)started in the background for no benefit.
I imagine that the number of people who use Ren'Py on Windows and also use Emacs with it is very small, probably it is just me. Similarly to on Linux, if the renpy executable is on PATH then it should just work (it should not matter about the .exe in the name), and if it is somewhere else the user would just configure the absolute path to it, so I don't think you have to do anything special here.
I have Emacs configured as the editor used by the launcher, by setting the environment variable |
e69b957 to
435f67b
Compare
I couldn't reproduce this on Linux.
If you managed to make the linter work on Windows then it works better than I expected. :-) I know almost nothing about the platform, and exactly nothing about running Emacs on Windows.
Yes, sure. This and other comments integrated. @morganwillcock Let me know if you have any other comments! |
435f67b to
d8f7e41
Compare
| (defun renpy--find-project-root () | ||
| "Find the Ren'Py project root by looking for a \"game/\" directory. | ||
| Return the full path to the \"game/\" directory if found, or nil if not." | ||
| (when-let (dir (locate-dominating-file default-directory "game")) | ||
| (expand-file-name "game" dir))) |
There was a problem hiding this comment.
I would suggest renaming this function to reflect the return value (it returns the game directory), and also explicitly describe the value as the game directory to make sure that this does not get confused with the concept of the project.el project root.
--- a/renpy.el
+++ b/renpy.el
@@ -2014,8 +2014,8 @@ defvar renpy--flymake-regex-list
(defvar-local renpy--flymake-proc nil
"Flymake process for the current buffer.")
-(defun renpy--find-project-root ()
- "Find the Ren'Py project root by looking for a \"game/\" directory.
+(defun renpy--find-project-game-directory ()
+ "Find the Ren'Py project \"game/\" directory.
Return the full path to the \"game/\" directory if found, or nil if not."
(when-let (dir (locate-dominating-file default-directory "game"))
(expand-file-name "game" dir)))
@@ -2041,11 +2041,11 @@ defun renpy--flymake-backend
REPORT-FN - function used to report diagnostics."
(unless (executable-find renpy-program)
(error "Cannot find `%s` executable" renpy-program))
- (let* ((root (renpy--find-project-root)))
- (if (not root)
+ (let* ((game-dir (renpy--find-project-game-directory)))
+ (if (not game-dir)
(progn
(flymake-log
- :error "Ren'Py project root not found for buffer %s" (buffer-name))
+ :error "Ren'Py game directory not found for buffer %s" (buffer-name))
(funcall report-fn nil))
;; Create a new Flymake subprocess for linting. If a previous process
;; exists, kill it first.
@@ -2055,7 +2055,7 @@ defun renpy--flymake-backend
(make-process
:name "renpy-flymake"
:buffer (generate-new-buffer "*renpy-flymake*")
- :command (list renpy-program root "lint")
+ :command (list renpy-program game-dir "lint")
:noquery t
:sentinel
(lambda (p _event)| (let (diags) | ||
| (dolist (re renpy--flymake-regex-list) | ||
| (while (re-search-forward re nil t) | ||
| (let* ((fname (match-string 1)) |
There was a problem hiding this comment.
This let* can be a let.
--- a/renpy.el
+++ b/renpy.el
@@ -2028,9 +2028,9 @@ defun renpy--parse-lint-buffer
(let (diags)
(dolist (re renpy--flymake-regex-list)
(while (re-search-forward re nil t)
- (let* ((fname (match-string 1))
- (line (string-to-number (match-string 2)))
- (msg (match-string 3)))
+ (let ((fname (match-string 1))
+ (line (string-to-number (match-string 2)))
+ (msg (match-string 3)))
(push (flymake-make-diagnostic
fname (cons line 0) nil :error msg)
diags))))| (let (diags) | ||
| (dolist (re renpy--flymake-regex-list) | ||
| (while (re-search-forward re nil t) | ||
| (let* ((fname (match-string 1)) |
There was a problem hiding this comment.
I would use filename instead of fname to avoid confusing it for "function name".
--- a/renpy.el
+++ b/renpy.el
@@ -2028,11 +2028,11 @@ defun renpy--parse-lint-buffer
(let (diags)
(dolist (re renpy--flymake-regex-list)
(while (re-search-forward re nil t)
- (let ((fname (match-string 1))
+ (let ((filename (match-string 1))
(line (string-to-number (match-string 2)))
(msg (match-string 3)))
(push (flymake-make-diagnostic
- fname (cons line 0) nil :error msg)
+ filename (cons line 0) nil :error msg)
diags))))
diags)))
| (defun renpy--find-project-root () | ||
| "Find the Ren'Py project root by looking for a \"game/\" directory. | ||
| Return the full path to the \"game/\" directory if found, or nil if not." | ||
| (when-let (dir (locate-dominating-file default-directory "game")) |
There was a problem hiding this comment.
This would be better as and-let* because when-let will be marked as obsolete in Emacs 31, and we are interested in the return value rather than a side-effect.
--- a/renpy.el
+++ b/renpy.el
@@ -2017,7 +2017,7 @@ defvar-local renpy--flymake-proc
(defun renpy--find-project-game-directory ()
"Find the Ren'Py project \"game/\" directory.
Return the full path to the \"game/\" directory if found, or nil if not."
- (when-let (dir (locate-dominating-file default-directory "game"))
+ (and-let* ((dir (locate-dominating-file default-directory "game")))
(expand-file-name "game" dir)))
(defun renpy--parse-lint-buffer (buffer)| REPORT-FN - function used to report diagnostics." | ||
| (unless (executable-find renpy-program) | ||
| (error "Cannot find `%s` executable" renpy-program)) | ||
| (let* ((root (renpy--find-project-root))) |
There was a problem hiding this comment.
This can be let instead of let*, but given what follows it is more direct to remove the not and use if-let* (if-let will be marked as obsolete in Emacs 31).
--- a/renpy.el
+++ b/renpy.el
@@ -2041,31 +2041,31 @@ defun renpy--flymake-backend
REPORT-FN - function used to report diagnostics."
(unless (executable-find renpy-program)
(error "Cannot find `%s` executable" renpy-program))
- (let* ((game-dir (renpy--find-project-game-directory)))
- (if (not game-dir)
- (progn
- (flymake-log
- :error "Ren'Py game directory not found for buffer %s" (buffer-name))
- (funcall report-fn nil))
- ;; Create a new Flymake subprocess for linting. If a previous process
- ;; exists, kill it first.
- (when (process-live-p renpy--flymake-proc)
- (kill-process renpy--flymake-proc))
- (setq renpy--flymake-proc
- (make-process
- :name "renpy-flymake"
- :buffer (generate-new-buffer "*renpy-flymake*")
- :command (list renpy-program game-dir "lint")
- :noquery t
- :sentinel
- (lambda (p _event)
- (when (memq (process-status p) '(exit signal))
- (unwind-protect
- (if (eq p renpy--flymake-proc)
- (let ((diags (renpy--parse-lint-buffer (process-buffer p))))
- (funcall report-fn diags))
- (flymake-log :warning "Obsolete Flymake process %s" p))
- (kill-buffer (process-buffer p))))))))))
+ (if-let* ((game-dir (renpy--find-project-game-directory)))
+ (progn
+ ;; If a previous process exists, kill it first.
+ (when (process-live-p renpy--flymake-proc)
+ (kill-process renpy--flymake-proc))
+ ;; Create a new process.
+ (setq renpy--flymake-proc
+ (make-process
+ :name "renpy-flymake"
+ :buffer (generate-new-buffer "*renpy-flymake*")
+ :command (list renpy-program game-dir "lint")
+ :noquery t
+ :sentinel
+ (lambda (p _event)
+ (when (memq (process-status p) '(exit signal))
+ (unwind-protect
+ (if (eq p renpy--flymake-proc)
+ (let ((diags (renpy--parse-lint-buffer
+ (process-buffer p))))
+ (funcall report-fn diags))
+ (flymake-log :warning "Obsolete Flymake process %s" p))
+ (kill-buffer (process-buffer p))))))))
+ (flymake-log
+ :error "Ren'Py game directory not found for buffer %s" (buffer-name))
+ (funcall report-fn nil)))
;;;; Modes.
|
|
||
| ;; Setup the Flymake backend. | ||
| (add-hook 'flymake-diagnostic-functions #'renpy--flymake-backend nil t) | ||
| ;; NOTE: No need to check buffer state at all as the linter works with all |
There was a problem hiding this comment.
I would remove the "NOTE: " part of the comment because that doesn't really add anything to the description of why the value is being set.
| (dolist (re renpy--flymake-regex-list) | ||
| (while (re-search-forward re nil t) |
There was a problem hiding this comment.
Since point never goes backwards, are you sure that by matching one of the regex patterns you are not moving past a potential match for the other pattern?
It would seem safer to just define a single pattern (i.e. put an (or in the rx form) and just match against that single pattern.
|
All good, will fix. I am on a trip so week so no access to a decent machine. |
This is a draft pull request of linting support through Flymake and the "renpy lint" command. Intended to be reviewed and improved. Tests included but some things are just hard to test whenever subprocesses are involved.
Overall this is a simple backend, nothing special here. There is one thing that worries me:
Most users work on Windows. What's the best way make sure it just works for them? How do people lint their projects on Windows?
I don't even have windows at hand, only linux.
@morganwillcock what do you think?