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

New erlang (treesitter) mode #9322

Open
wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Conversation

dgud
Copy link

@dgud dgud commented Jan 15, 2025

Brief summary of what the package does

New erlang (treesitter) mode

Currently only fixes font-locking and relies on the "old" erlang mode for everything else.

Direct link to the package repository

https://github.com/erlang/emacs-erlang-ts

Your association with the package

maintainer, author

Relevant communications with the upstream package maintainer

None needed

Checklist

** package-lint reports errors for function names but they are intentionally there for overriding the
the erlang mode functions.

@riscy
Copy link
Member

riscy commented Jan 19, 2025

Thanks for this. Quick first pass -

erlang-ts.el with package-lint 20250108.1407:

5 issues found:
26:19: warning: "" is not a valid version. MELPA will handle this, but other archives will not.
71:0: error: "erlang-font-lock-level-1" doesn't start with package's prefix "erlang-ts".
76:0: error: "erlang-font-lock-level-2" doesn't start with package's prefix "erlang-ts".
81:0: error: "erlang-font-lock-level-3" doesn't start with package's prefix "erlang-ts".
86:0: error: "erlang-font-lock-level-4" doesn't start with package's prefix "erlang-ts".

It's probably a good idea to add a version, and I see you commented:

package-lint reports errors for function names but they are intentionally there for overriding the the erlang mode functions.

and I see from your code that this is "So the menus (and functions) work as expected". I think yours is the only ts package I've seen that had this requirement, and I usually try to be pretty intolerant about people dipping their toes outside their namespaces - even a closely related one like this.

Could you describe what's going on that's causing the functions (which?) to work unexpectedly?

@dgud
Copy link
Author

dgud commented Jan 20, 2025

It's probably a good idea to add a version

Hmm, ok, maintaining the "old" erlang mode for 15 years, and there the version have been updated,
maybe twice.
So I thought it would be better if it was updated by the package-manager.

Could you describe what's going on.

I don't have time to completely re-write all the functionality from the old mode in the new erlang-ts mode now,
I will do that bit by bit when time permits.

So erlang-ts mode (currently) loads and re-uses the old erlang mode for everything except syntax-highlighting which
is overloaded via the treesitters font-lock functionality.

The old erlang mode installs menus where the user can change syntax-highlighting level, those menu entries call
erlang-font-lock-level-X functions which I have overloaded in the erlang-ts mode so the user can change syntax-highlighting level from the menu even when using erlang-ts mode.

I'm not emacs-lisp guy, just a user and the unlucky guy in the maintaining lottery :-)
do you have better suggestion of how overload the functionality?

@riscy
Copy link
Member

riscy commented Jan 26, 2025

So I thought it would be better if it was updated by the package-manager.

That's fair - I think some people even just leave it at version 0.0. My only (weak) counter would be that I guess people sometimes find elisp files lurking on wikis and old archives, and in that case seeing the versions could be interesting (if not that useful). I would recommend avoiding :version-regexp in your recipe - semantic versioning should be easily parsed by the package-build machinery.

do you have better suggestion of how overload the functionality?

I understand the time constraint (and thank you, btw, for your maintainership). I think a better pattern would be to create advice functions inside your namespace:

(defun erlang-ts--font-lock-level-1 (func &rest args)
  "To be used as :around advice."
  (if erlang-ts-mode
      (erlang-ts-set-font-lock-level 1)
    (apply func args)))

Then, the first time your mode is toggled on, you could run:

(advice-add #'erlang-font-lock-level-1 :around #'erlang-ts--font-lock-level-1)

(this is idempotent so it's okay if it gets called repeatedly)

Finally, I'd recommend you add a erlang-ts-unload-function function so that if a user did run unload-feature (q.v.) the advice would be removed too, using:

(advice-remove #'erlang-font-lock-level-1 #'erlang-ts--font-lock-level-1)

@dgud
Copy link
Author

dgud commented Jan 26, 2025

I have added (and tagged) a version 0.1 last week,

I will test your other suggestion next week, need to read up emacs-lisp a bit.

Thanks for taking time and making suggestions of how to solve it.

@dgud
Copy link
Author

dgud commented Jan 27, 2025

To be able to get rid of all warnings I had to do:

(defun erlang-ts--font-lock-level-1 (func &rest args)
  "To be used as :around advice.
FUNC with ARGS will be called if `erlang-ts-mode' is not active."
  (if (derived-mode-p 'erlang-ts-mode)
      (erlang-ts-set-font-lock-level 1)
    (apply func args)))

And I also added a unload function

(defun erlang-ts-unload-function ()
  "Used by `unload-feature'."

  (while (rassoc 'erlang-ts-mode auto-mode-alist)
    (setq auto-mode-alist
          (assq-delete-all (car (rassoc 'erlang-ts-mode auto-mode-alist))
                           auto-mode-alist)))

  (advice-remove #'erlang-font-lock-level-1 #'erlang-ts--font-lock-level-1)
  (advice-remove #'erlang-font-lock-level-2 #'erlang-ts--font-lock-level-2)
  (advice-remove #'erlang-font-lock-level-3 #'erlang-ts--font-lock-level-3)
  (advice-remove #'erlang-font-lock-level-4 #'erlang-ts--font-lock-level-4))

And things seems to work as expected.

@riscy
Copy link
Member

riscy commented Feb 2, 2025

Thanks! Here's the latest (maybe your default branch wasn't updated? Also I want to emphasize that the version-regexp is probably not required).

erlang-ts.el with package-lint 20250108.1407:

4 issues found:
71:0: error: "erlang-font-lock-level-1" doesn't start with package's prefix "erlang-ts".
76:0: error: "erlang-font-lock-level-2" doesn't start with package's prefix "erlang-ts".
81:0: error: "erlang-font-lock-level-3" doesn't start with package's prefix "erlang-ts".
86:0: error: "erlang-font-lock-level-4" doesn't start with package's prefix "erlang-ts".

erlang-ts.el with melpazoid:

- `;;; Commentary` is much wider than 80 characters

⸺ Package and license:

  • Avoid :version-regexp in recipes except in unusual cases
  • In case you haven't, ensure GitHub release v0.1 is up-to-date with your current code and Package-Version

@dgud
Copy link
Author

dgud commented Feb 2, 2025

Check again, I had pushed to my "private" repo and not the official/upstream one, sigh.

Pushed it to upstream now.

Avoid :version-regexp in recipes except in unusual cases

What does that mean, that you want use the github tag?

In case you haven't, ensure GitHub release v0.1 is up-to-date with your current code and Package-Version

I will tag the release when you are happy :-)

@riscy
Copy link
Member

riscy commented Feb 2, 2025

What does that mean, that you want use the github tag?

Your recipe is

(erlang-ts
    :fetcher github
    :repo "erlang/emacs-erlang-ts"
    :version-regexp "v%v")

but :version-regexp is probably not necessary to specify here since you're already following a convention that package-build can figure out.

@dgud dgud force-pushed the dgud/add-erlang-ts branch from 396ad2d to feb757e Compare February 3, 2025 08:18
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