Re: New package: haskell-ts-mode

[Philip Kaludercic <philipk@posteo.net>]

[-- Attachment #1: Type: text/plain, Size: 685 bytes --]

Pranshu Sharma <sirsharmapranshu@gmail.com> writes:

> Hello all
>
> haskell-ts-mode is a major mode for haskell that uses treesitter to provide
> features such as indentation and colouring. The mode can be found at
> pranshu/haskell-ts-mode:
> A haskelll mode that uses treesitter - Codeberg.org
> <https://codeberg.org/pranshu/haskell-ts-mode>.

First of all, the spacing and indentation is inconsistent.  Please
address that, and perhaps add a directory local option to enforce
indentation with spaces to ensure a consistent look.

I cannot comment on the tree-sitter stuff since I don't really
understand it, but otherwise I have a few comments that I would like you
to consider:


[-- Attachment #2: Type: text/plain, Size: 7685 bytes --]

diff --git a/haskell-ts-mode.el b/haskell-ts-mode.el
index 4ba7f42..391db66 100644
--- a/haskell-ts-mode.el
+++ b/haskell-ts-mode.el
@@ -2,7 +2,6 @@
 
 ;; Copyright (C) 2024  Pranshu Sharma
 
-
 ;; Author: Pranshu Sharma <pranshusharma366 at gmail>
 ;; URL: https://codeberg.org/pranshu/haskell-ts-mode
 ;; Package-Requires: ((emacs "29.3"))
@@ -23,6 +22,7 @@
 ;; along with this program.  If not, see <https://www.gnu.org/licenses/>.
 
 ;;; Commentary:
+
 ;; This is a WIP mode that uses treesitter to provide all the basic
 ;; major mode stuff, like indentation, font lock, etc...
 
@@ -31,6 +31,7 @@
 (require 'comint)
 (require 'treesit)
 
+;; From what I understand this is necessary for tree-sitter modes, but can you at least also add the ARGLIST argument.  Also, some functions such as `treesit-induce-sparse-tree' appear not to be used?
 (declare-function treesit-parser-create "treesit.c")
 (declare-function treesit-induce-sparse-tree "treesit.c")
 (declare-function treesit-node-child "treesit.c")
@@ -46,10 +47,11 @@
     (otherwise signature)))
 
 (defvar haskell-ts-use-indent t
-  "Set to nil if you don't want to use Emacs indent.")
+  "Set to nil if you don't want to use Emacs indent.") ;rephrase this would the double negation
 
 (defvar haskell-ts-font-lock-level 4
   "Level of font lock, 1 for minimum highlghting and 4 for maximum.")
+;; both of these variables are actually user options and should be declared using `'defcustom'!
 
 (defvar haskell-ts-prettify-symbols-alits
       '(("\\" . "λ")
@@ -60,6 +62,7 @@
 	("<=" . "≥")
 	(">=" . "≤")))
 
+;; Checkdoc complains that 
 (defun haskell-ts-font-lock ()
     (treesit-font-lock-rules
        :language 'haskell
@@ -157,7 +160,7 @@
 	   ((parent-is "apply") parent -1)
 	   ((node-is "quasiquote") grand-parent 2)
 	   ((parent-is "quasiquote_body") (lambda (_ _ c) c) 0)
-	   ;; Do Hg
+	   ;; Do Hg  ;; what does "Hg" mean?
 	   ((lambda (node parent bol)
 	      (let ((n (treesit-node-prev-sibling node)))
 		(while (string= "comment" (treesit-node-type n))
@@ -232,6 +235,7 @@
 
 ;; Copied from haskell-tng-mode, changed a bit
 (defvar haskell-ts-mode-syntax-table
+  ;; Should this be wrapped in an `eval-when-compile'?
       (let ((table (make-syntax-table)))
 	(map-char-table
 	 (lambda (k v)
@@ -241,7 +245,7 @@
                (modify-syntax-entry k "_" table))))
 	 (char-table-parent table))
 	;; whitechar
-	(seq-do
+	(seq-do				;seq-do has a relatively high overhead, try to avoid it
 	 (lambda (it) (modify-syntax-entry it " " table))
 	 (string-to-list "\r\n\f\v \t"))
 	;; ascSymbol
@@ -268,46 +272,40 @@
 	(modify-syntax-entry ?\{  "(}1nb" table)
 	(modify-syntax-entry ?\}  "){4nb" table)
 	(modify-syntax-entry ?-  "_ 123" table) ;; TODO --> is not a comment
-	(seq-do
-	 (lambda (it) (modify-syntax-entry it ">" table))
-	 (string-to-list "\r\n\f\v"))
+	(dolist (c (string-to-list "\r\n\f\v")) ;though unrolling wouldn't be bad either
+	  (modify-syntax-entry c ">" table))
+	
 	table))
 
+(defun haskell-ts-imenu-name-function (check-func)
+  (lambda (node)
+    (if (funcall check-func node)
+	(haskell-ts-defun-name node)
+      nil)))
 
-(defmacro haskell-ts-imenu-name-function (check-func)
-  `(lambda (node)
-      (if (funcall ,check-func node)
-	  (haskell-ts-defun-name node)
-	nil)))
-
-(defun haskell-ts-indent-para()
+(defun haskell-ts-indent-para ()
   "Indent the current paragraph."
   (interactive)
-  (save-excursion
-    (backward-paragraph)
-    (let ((p (point)))
-      (forward-paragraph)
-      (indent-region p (point)))))
+  (when-let ((par (bounds-of-thing-at-point 'paragraph)))
+    (indent-region (car par) (cdr par))))
 
 (defvar haskell-ts-mode-map
   (let ((km (make-sparse-keymap)))
     (define-key km (kbd "C-c C-c") 'haskell-ts-compile-region-and-go)
     (define-key km (kbd "C-c C-r") 'haskell-ts-run-haskell)
-    (define-key km (kbd "C-M-q") 'haskell-ts-indent-para)
+    (define-key km (kbd "C-M-q") 'haskell-ts-indent-para) ;is this necessary when `prog-fill-reindent-defun' is bound to M-q?
     km)
-  "Map for haskell-ts-mode")
+  "Map for haskell-ts-mode.")
 
 ;;;###autoload
 (define-derived-mode haskell-ts-mode prog-mode "haskell ts mode"
   "Major mode for Haskell files using tree-sitter."
-  :syntax-table haskell-ts-mode-syntax-table
-  :interactive t
   (unless (treesit-ready-p 'haskell)
     (error "Tree-sitter for Haskell is not available"))
   (treesit-parser-create 'haskell)
   (setq-local treesit-defun-type-regexp "\\(?:\\(?:function\\|struct\\)_definition\\)")
   ;; Indent
-  (and haskell-ts-use-indent
+  (and haskell-ts-use-indent		;do you mean `when'?
        (setq-local treesit-simple-indent-rules haskell-ts-indent-rules)
        (setq-local indent-tabs-mode nil))
   ;; Comment
@@ -316,21 +314,21 @@
   (setq-local comment-start-skip "\\(?: \\|^\\)-+")
   ;; Elecric
   (setq-local electric-pair-pairs
-	      (list (cons ?` ?`) (cons ?\( ?\)) (cons ?{ ?}) (cons ?\" ?\") (cons ?\[ ?\])))
+	      '((?` . ?`) (?\( . ?\)) (?{ . ?}) (?\" . ?\") (?\[ . ?\])))
   ;; Nav
-  (setq-local treesit-defun-name-function 'haskell-ts-defun-name)
+  (setq-local treesit-defun-name-function #'haskell-ts-defun-name)
   (setq-local treesit-defun-type-regexp "function")
   (setq-local prettify-symbols-alist haskell-ts-prettify-symbols-alits)
   ;; Imenu
   (setq-local treesit-simple-imenu-settings
 	      `((nil haskell-ts-imenu-func-node-p nil
-		     ,(haskell-ts-imenu-name-function 'haskell-ts-imenu-func-node-p))
+		     ,(haskell-ts-imenu-name-function #'haskell-ts-imenu-func-node-p))
 		("Signatures.." haskell-ts-imenu-sig-node-p nil
-		 ,(haskell-ts-imenu-name-function 'haskell-ts-imenu-sig-node-p))
+		 ,(haskell-ts-imenu-name-function #'haskell-ts-imenu-sig-node-p))
 		("Data..." haskell-ts-imenu-data-type-p nil
 		 (lambda (node)
 		   (treesit-node-text (treesit-node-child node 1))))))
-  ;; font-lock.
+  ;; font-lock
   (setq-local treesit-font-lock-level haskell-ts-font-lock-level)
   (setq-local treesit-font-lock-settings (haskell-ts-font-lock))
   (setq-local treesit-font-lock-feature-list
@@ -379,23 +377,32 @@
 
 (defun haskell-ts-run-haskell()
   (interactive)
-  (when (not (comint-check-proc "*haskell*"))
-    (set-buffer (apply (function make-comint)
-		       "haskell" "ghci" nil `(,buffer-file-name))))
-  (pop-to-buffer-same-window "*haskell*"))
+  (pop-to-buffer-same-window		;really in the same window?
+   (or
+    (comint-check-proc "*haskell*")
+    (make-comint "*haskell* repl" "ghci" nil buffer-file-name))))
+
 
 (defun haskell-ts-haskell-session ()
   (get-buffer-process "*haskell*"))
 
+(defvar eglot-server-programs)		;to avoid the byte-compiler error
 (defun haskell-ts-setup-eglot()
   (when (featurep 'eglot)
-   (add-to-list 'eglot-server-programs
+    ;; Eglot was added to the core along with tree-siter, so there is
+    ;; no case where tree-sitter is available, but eglot is not.
+    (add-to-list 'eglot-server-programs
 	       '(haskell-ts-mode . ("haskell-language-server-wrapper" "--lsp")))))
 
+;; Note that you write (eval-when-load 'eglot
+;; (haskell-ts-setup-eglot)) in README.org, but that doesn't do what
+;; you want it to.  That will modify eglot-server-programs and
+;; evaluate the result when eglot is loaded.  You presumably wanted to
+;; use `with-eval-after-load'?
+
 (when (treesit-ready-p 'haskell)
   (add-to-list 'auto-mode-alist '("\\.hs\\'" . haskell-ts-mode)))
 
 (provide 'haskell-ts-mode)
 
 ;;; haskell-ts-mode.el ends here
-

[-- Attachment #3: Type: text/plain, Size: 1091 bytes --]


(Also, a general question, do you use LLMs to generate some of the code?)

>
> The attached patch is for the elpa repo
> Thanks,
> Pranshu
> diff --git a/elpa-packages b/elpa-packages
> index 137fef0348..451a67d395 100644
> --- a/elpa-packages
> +++ b/elpa-packages
> @@ -368,6 +368,9 @@
>   (gtags-mode		:url "https://github.com/Ergus/gtags-mode")
>   (guess-language	:url "https://github.com/tmalsburg/guess-language.el"
>    :merge t)
> + (haskell-ts-mode       :url "https://codeberg.org/pranshu/haskell-ts-mode"
> +  :doc "README.org"

Are you sure that you want the README to be installed as a manual?

> +  :ignored-files ("*.png" "LICENSE"))

You can list the files you wish to ignore in an .elpaignore file in your
repository; its preferable to using :ignored-files as it is more
flexible and easier to adjust if something changes on your end.

>   (hcel			:url "https://g.ypei.me/hc.el.git")
>   (heap			:url nil) ;"http://www.dr-qubit.org/git/predictive.git"
>   (hiddenquote		:url "https://gitlab.com/mauroaranda/hiddenquote/hiddenquote")
>

-- 
	Philip Kaludercic on peregrine