Re: Merge haskell-ts-mode in upstream

[Pranshu Sharma via "Emacs development discussions." <emacs-devel@gnu.org>]

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

Philip Kaludercic <philipk@posteo.net> writes:

> I see that you have started a new repository.  Do you want us to mirror
> your changes with all the commit history, or are you OK with us just
> copying over the coded periodically whenever you want to update the code?

Ok, it seems like I'll be sticking to git one anyway, since
git-hg-bridge or something doesn't work.  I am fine with using the old
repo.


>> ;; This is a major mode that uses treesitter to provide all the basic
>> ;; major mode stuff, like indentation, font lock, etc...
>
> I am not clear about this aspect of -ts-modes, but wouldn't it make
> sense to link to the grammar that you tested the major mode against?

Thanks, I added it.  For now I'll intentionally not state the version,
as it works with the latest, and I don't know how back it goes until it doesn't.

>
> You can drop the :group declarations, they automatically fall back to
> the last `defgroup'.

Wow, that's handy.

>>   :type 'boolean)
>>
>> (defcustom haskell-ts-font-lock-level 4
>>   "Level of font lock, 1 for minimum highlghting and 4 for maximum."
>>   :group 'haskell-ts-mode
>>   :type 'integer)
>
> It might even make sense to use a choice here, as not all integer values
> are valid.

From the customize manual, using the :options is only avialiable with
few :types, like hook and plist.  I don't think it works for numbers.

>> (defvar haskell-ts-prettify-symbols-alist ...)
>
> Would it make sense for this to be a defconst?
>

I don't think that's neccsary, the user might want to add/remove some
stuff

>> (defun haskell-ts--stand-alone-parent (_ parent bol)
>>   (save-excursion
>>     (goto-char (treesit-node-start parent))
>>     (let ((type (treesit-node-type parent)))
>>       (if (and (not bol)
>> 	       (or (looking-back "^[ \t]*" (line-beginning-position))
>> 		   (seq-some
>> 		    (lambda (kw) 
>> 		      (string= type kw))
>> 		    '("when" "where" "do" "let" "local_binds" "function"))))
>
> Why are you using `seq-some' and not a faster primitive like `member'?
> In this context, the behaviour should be the same, unless you are
> expecting that `type' can be something else than a string?

Thanks, change applied. 

> You probably want to sharp-quote these.
Yep
>
>> 		   (n (funcall func	 node)))
>                                    ^
>                                    is the tab here intentional?
thanks, dealt with.
>>        ((node-is "^comment$")
>> 	;; Indenting comments by priorites:
>> 	;; 1. next relevent sibling if exists
>> 	;; 2. previous relevent sibling if exists
>> 	;; 3. parent
>> 	;; (relevent means type not it haskell-ts--ignore-types)
>> 	(lambda (node parent _)
>> 	  (if-let ((next-sib (funcall ,p-sib node nil)))
>> 	      (treesit-node-start next-sib)
>> 	    (if-let ((prev-sib (funcall ,p-prev-sib node nil nil)))
>> 		prev-sib
>> 	      (treesit-node-start parent))))
>
> It might be fun to rewrite this using pcase...

It indeed was, wow

>> 	0)
>>        ;; Backup
>>        (catch-all parent 2)))))
>>
>> ;; Copied from haskell-tng-mode, changed a bit
>>
>
> Consider wrapping this in a `eval-when-compile' and perhaps re-writing
> it with `dolist'.

Good idea, that made the code way cleaner

> You should clean up the indentation, you seem to have tabs in the code
> made the code intended in a harder to read way on my machine.

Ok, I changed the tab width and indented whole page.  tell me if still a
problem.

>>
>> (defun haskell-ts-indent-defun (pos)
>>   "Indent the current function."
>>   (interactive "d")
>>   (let ((node (treesit-node-at pos)))
>>     (while (not (string-match
>> 		 "^declarations$\\|haskell"
>> 		 (treesit-node-type (treesit-node-parent node))))
>>       (setq node (treesit-node-parent node)))
>>     (indent-region (treesit-node-start node) (treesit-node-end node))))
>
> Why is this function necessary, if we already have the general commands
> for indenting a defun?  If it is, it should probably be explained in the
> docstring.

Haskell functions are not paren wrapped(opptionally they are), so when I
tested those functions don't work. C-M-h works, but the indentation is a
little different in treesitter based mode, it works differently in
incomplete sentences.  Meaning newline-indent would rarley be the final
indentation of any expression.  I think the reason is too techical to
include in docstring.

>> (defvar haskell-ts-mode-map
>>   (define-keymap
>>     "C-c C-c" 'haskell-ts-compile-region-and-go
>>     "C-c C-r" 'haskell-ts-run-haskell
>>     "C-M-q" 'haskell-ts-indent-defun)
>>   "Map for haskell-ts-mode.")
>
> I am guessing this should be a `defvar-keymap'.
>

Thanks, TIL

>>
>> (defun haskell-ts-compile-region-and-go (start end)
>>   "Compile the text from START to END in the haskell proc."
>>   (interactive "r")
>>   (let ((hs (haskell-ts-haskell-session)))
>>     (comint-send-string hs ":{\n")
>>     (comint-send-region hs start end)
>
> You should probably do something to ensure that the interval doesn't
> contain ":}"?

I don't get what you mean.

> I assume that this is something ghci specific, right?

Yeah, ghci requires special wrapping for multiline stuff.  This makes
sense, as both the functions are valid:
f x = x + 1
f x = n + 1
  where n = x + 2
ghci not smart enough to tell that a 'where' might come at end.

>>     (comint-send-string hs "\n:}\n")))
>>
>> (defun haskell-ts-run-haskell()
>>   (interactive)
>>   (pop-to-buffer-same-window
>>    (if (comint-check-proc "*haskell*")
>>        "*haskell*"
>>      (make-comint "haskell" "ghci" nil buffer-file-name))))
>
> 1. Would it make sense to have a user option that allows customising
>    this, in case someone uses Cabal, Stack or whatever else?

I have a lot to say about this: Haskell's eco system is something that
will have you scratching your head when sleeping.  Trying to support
these utilities is soft suacide.  This package provides a simple ghci
wrapper, and I don't want it to become too complex, and inturn hard to
maintain(these tools are diddy when it comes to backward compatibity)
and understand.  If someone wants to make a complex ghci wrapper which
can do all this, they could(but shouldn't, for the sake of mental
health).  This mode attempts ot provide syntax+indentation+imenu+
someminorstuff *fullstop*, not provide the whole hassle-less haskell
development experiance(even if it existed).  The current model is that
people just do `import`in ghc. iirc, no fancy cabal/stakc program exists
to start ghci with all the app loaded.  To haskell-ts-mode, haskell goes
as far as ghc, cabal and stack don't exist.

Wait, now that I read it, you meant an option to specify ghci
path(added).  I went on a rant as soon as I saw cabal, oh well...

> 2. The regular haskell-mode has a `run-haskell' command that creates the
>    same buffer name.  Assuming that it is not that unusual for people to
>    have both installed, when trying out the one or the other, shouldn't
>    you take some precaution to detect that scenario?

yes, that makes sense, I changed buffer name to *Inferior Haskell*.
Initially I wanted to name the functino run-haskell for consistancy(eg
run-php (in php-ts), run-scheme, etc...), but this is fine.

I attach new file, I have also pushed changes to codebrg repo.


[-- Attachment #2: haskell-ts-mode.el --]
[-- Type: application/emacs-lisp, Size: 15677 bytes --]

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



-- 
Pranshu Sharma <https://p.bauherren.ovh>