Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Don't fail silently when grammar is missing #1

Merged
merged 1 commit into from
Mar 31, 2024
Merged

Conversation

purcell
Copy link
Contributor

@purcell purcell commented Sep 5, 2023

These conditional clauses are found in some -ts modes, but only in the case where the parent mode is the non-ts version of the mode: in such cases, the clause serves to quietly "downgrade" the mode to the non-ts version when the grammar is missing, so that the -ts mode can be enabled unconditionally in auto-mode-alist.

In cases where the parent mode is just prog-mode, like noir-ts-mode, the -ts mode is non-functional without the grammar and should throw an error instead.

As a related note, it is unclear how best to handle the ###autoload-ed addition to auto-mode-alist, since we'd expect to be able to install the same package config in Emacsen with and without tree-sitter. It's probably safest to guard the auto-mode-alist expression in a (when (treesit-read-p 'lang) ...) form too.

(It looks like a couple of whitespace fixes found their way into the commit too, sorry.)

These conditional clauses _are_ found in some `-ts` modes, but only in the case where the parent mode is the non-`ts` version of the mode: in such cases, the clause quietly "downgrades" the mode to the non-`ts` version when the grammar is missing, so that the `-ts` mode can be enabled unconditionally in `auto-mode-alist`.

In other cases, like `noir-ts-mode`, the mode is non-functional without the grammar and should throw an error instead.

It is unclear how best to handle the `###autoload`-ed addition to `auto-mode-alist`, since we'd expect to be able to install the same package config in Emacsen with and without tree-sitter. It's probably safest to guard the `auto-mode-alist` expression in a `(when (treesit-read-p 'lang) ...)` form too.
@hhamud
Copy link
Owner

hhamud commented Sep 5, 2023

Hey Purcell. Thanks for the contribution.

I have another noir mode here that is based on the Rust mode and I was thinking about a way to combine the ts-mode and non-ts mode packages together but I haven't had enough time to think it through and implement them.

Any suggestions would be welcome.

@purcell
Copy link
Contributor Author

purcell commented Sep 5, 2023

Yeah, then it sounds like you could probably follow the pattern of, say python-mode and python-ts-mode in the emacs standard library: make the ts mode package depend on the non-ts package, and use noir-mode as the parent of noir-ts-mode. If you did that, you'd keep the when clause that this PR removes.

@hhamud
Copy link
Owner

hhamud commented Sep 5, 2023

I think what I can do is merge the above changes when you've included this change

It's probably safest to guard the auto-mode-alist expression in a (when (treesit-read-p 'lang) ...) form too.

and then when I've updated the noir package down the line to update both packages.

@purcell
Copy link
Contributor Author

purcell commented Sep 5, 2023

That makes sense!

@hhamud hhamud merged commit eb399cc into hhamud:main Mar 31, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants