Re: [PATCH] feat: add markdown-ts-mode

["Jostein Kjønigsen" <jostein@secure.kjonigsen.net>]

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

Some comments so far about *how* the mode should correctly be added to repo, so I won't delve into that.

Instead, I like the idea behind having built in modes for stuff like this. So I went ahead and tested the real-world functionality of the major-mode on the files I have :)

First I tried to install the grammar using the default for treesit-install-grammar, and that failed. I then tweaked some settings, and I think I did the right thing?

Repo: https://github.com/tree-sitter-grammars/tree-sitter-markdown
Subfolder for compilation: tree-sitter-markdown/src

After installing this grammar using , I can compile/evaluate the provided major-mode (but not before!). I think most other TS-based major-modes in Emacs have a way to handle this more gracefully, and that should probably be addressed here too?

Now after compiling it and activating it... Im getting some confusing behaviour on existing files:

1. No syntax-highlighting at all. Not on new, nor existing buffers. What I am doing wrong here? :)
2. Newline chars in modeline to tell me my current section (# Deployment^J). Is this major-mode somehow linebreak-type sensitive? IMO that's not a great thing.
3. No imenu? Imenu really would make this nicer to use, but not really worth looking into until other issues are resolved.

The contrast to the version you say you've already published on MELPA is quite significant, where from what I can tell... 

In that mode, literally everything works. Are there any reason you can't submit that version instead of this modified version?

Also: Please don't consider these comments in a dissuading manner. These are meant as constructive criticism, because I want Emacs to have these things, but then they need to be good enough :)


—
Kind Regards
Jostein Kjønigsen

> On 20 Apr 2024, at 08:44, Eli Zaretskii <eliz@gnu.org> wrote:
> 
>> From: Rahul Martim Juliato <rahuljuliato@gmail.com>
>> Date: Sat, 20 Apr 2024 00:23:45 -0300
>> 
>> I've been using Emacs without any extra packages as an educational
>> experiment after years of package hording.
>> 
>> 
>> One of the few things I've been missing is a way of displaying some sort
>> of syntax highlight for markdown documents.
>> 
>> 
>> It feels a bit frustrating opening a README.md file with a single face
>> color, since Emacs from the box can handle so much.  I am sure others
>> might have shared this feeling before.
>> 
>> 
>> This patch is a modified version of a package I've recently published on
>> MELPA, screenshots are available here:
>> https://github.com/LionyxML/markdown-ts-mode
>> 
>> 
>> It is a very basic mode that provides syntax highlight and imenu support
>> using treesitter grammar from
>> https://github.com/tree-sitter-grammars/tree-sitter-markdown
>> 
>> 
>> The idea here is to provide minimal support to Emacs and continue
>> building up features in the future.
>> 
>> 
>> It is the first time I contribute to Emacs devel, so please let me know
>> iif I did something wrong or anything is not at the expected standards.
> 
> Thanks, please see a couple of comments below.  I've CC'ed Yuan as
> well, in case he has comments.
> 
>>> From 364f61b03d601d2cb3aeb1687da2d1b2a232474c Mon Sep 17 00:00:00 2001
>> From: Rahul Martim Juliato <rahul.juliato@gmail.com>
>> Date: Fri, 19 Apr 2024 23:21:20 -0300
>> 
>> ---
>> etc/NEWS                           |   5 ++
>> lisp/progmodes/markdown-ts-mode.el | 106 +++++++++++++++++++++++++++++
>> 2 files changed, 111 insertions(+)
>> create mode 100644 lisp/progmodes/markdown-ts-mode.el
> 
> Please accompany the patches with a ChangeLog-style commit log
> message; see CONTRIBUTE for the details.  In this case, you'd need a
> very minimal one, something like:
> 
>  * lisp/progmodes/markdown-ts-mode.el: New file.
>  * etc/NEWS: Announce the new mode.
> 
>> +---
>> +*** New major mode 'markdown-ts-mode'.
>> +A major mode based on the tree-sitter library for editing Markdown files.
> 
> How about mentioning this in the user manual as well?  It doesn't have
> to be anything more than just the name of the mode.
> 
>> diff --git a/lisp/progmodes/markdown-ts-mode.el b/lisp/progmodes/markdown-ts-mode.el
> 
> Should this mode live in lisp/textmodes/ instead?  Markdown is AFAIU a
> mode for text with markup, it isn't a programming language.
> 
>> +(defun markdown-ts-setup ()
>> +  "Setup treesit for `markdown-ts-mode'."
>> +  (setq-local treesit-font-lock-settings markdown-ts--treesit-settings)
>> +  (treesit-major-mode-setup))
>> +
>> +;;;###autoload
>> +(define-derived-mode markdown-ts-mode fundamental-mode "markdown[ts]"
>> +  "Major mode for editing Markdown using tree-sitter grammar."
>> +  (setq-local font-lock-defaults nil
>> +	      treesit-font-lock-feature-list '((delimiter)
>> +					       (paragraph)))
> 
> I wonder whether this mode should inherit from Text mode instead, and
> consequently have some text-related commands, perhaps aided by the
> tree-sitter grammar?  WDYT?  We could, of course, add commands later,
> but the decision to have Text mode as the parent of this one should be
> made now.
> 


[-- Attachment #2: Type: text/html, Size: 7629 bytes --]