Definitions don't stand out

[monnier]

The font-lock rules put the same face (font-lock-function-name-face) on function definitions as on function calls.
It makes it harder to see the definitions at a glance, and does not follow the usual practice of most other major modes I use.
Personally I'd be happy to see function calls in the default face, but you really care about using a different face, font-lock-function-call-face would be a good choice.

[josteink]

Hey Stefan.

Thanks for the bug-report. I haven’t verified, but you are probably right.

Just to be clear, is this bug report about the «legacy» typescript-mode and a fix wanted there? Or are you using the new typescript-ts-mode?

[monnier]

Yes, it's about typescript-mode, not typescript-ts-mode (maybe that applies there as well, but I haven't bothered to check).

[josteink]

Right. In that case, I'll have to confess I've stopped actively maintaining typescript-mode, and right now I'm personally only using typescript-ts-mode.

Basically what you're suggesting is that we should improve the parsing to be able to distinguish between what is a method-definition and what is a method-call, in a pretty complicated and flexible language.

But the reason we decided to go with tree-sitter for typescript in the first place, was because the grammar was getting too complex to manage in Elisp alone.

So based on that, I won't try to improve this major-mode or do anything active to try to get this issue closed.

On the flip side, if someone want's to contribute a patch/PR, I will be happy test review, test and merge.

Sounds fair? (Sorry about the delay)

[monnier]

Basically what you're suggesting is that we should improve the parsing to be able to distinguish between what is a method-definition and what is a method-call, in a pretty complicated and flexible language.

AFAICT, the font-lock rules which highlight definitions are already distinct from those that highlight calls, so it looks like a fairly simple change. Of course, maybe I'm missing something since I'm not knowledgeable about Typescript syntax. FWIW, I've used the patch below and it seemed to work. I don't code in Typescript (just occasionally need to review some Typescript code), so I don't know how well it works "in real life". The patch does more than just change that face: - Prefer ' over list+cons. - Quote face names instead of relying on the `font-lock*-face` variables (which are marked obsolete in Emacs-31). - Prefer `funcall` to `eval` (which also fixes a bug when the code uses `lexical-binding`). - Fix a "\[\]" in a regexp which is read as "[]" but I believe is meant to be a regexp that matches the string "[]" and so should be written "\\[\\]". This is in the penultimate hunk. - In the last hunk: change `font-lock-function-name-face` into `font-lock-function-call-face` at two places. Stefan

[josteink]

It seems like you've accidentally removed/deleted the patch?

But if you have a patch ready, just create a PR for it, and I'll get it merged if it looks reasonable!

[monnier]

It seems like you've accidentally removed/deleted the patch?

No, it's github which drops it on the floor, which is why I put you a `Cc:`, so you should have gotten the patch via email.

[josteink]

Sorry to be a bonehead about this, but I've tried all the different ways I can imagine to manually apply the patch, but I can't get it to merge cleanly.

$ git apply typescript-mode.patch
error: patch failed: typescript-mode.el:1
error: typescript-mode.el: patch does not apply
$ patch typescript-mode.el  typescript-mode.patch
patching file typescript-mode.el
12 out of 12 hunks failed--saving rejects to typescript-mode.el.rej
$ patch <typescript-mode.patch
patching file typescript-mode.el
12 out of 12 hunks failed--saving rejects to typescript-mode.el.rej
$

I really appreciate these patches though, so it would really help me out if you could send those patches as a PR here on Github, or tell me what I'm doing wrong 😄

[monnier]

````bash $ git apply typescript-mode.patch error: patch failed: typescript-mode.el:1 error: typescript-mode.el: patch does not apply $ patch typescript-mode.el typescript-mode.patch patching file typescript-mode.el 12 out of 12 hunks failed--saving rejects to typescript-mode.el.rej $ patch <typescript-mode.patch patching file typescript-mode.el 12 out of 12 hunks failed--saving rejects to typescript-mode.el.rej $ ````

Hmm... not sure what's going on. I just now pushed the patch after committing it to the `scratch/typescript-mode` branch on `nongnu.git`. You can fetch it with git fetch git://git.sv.gnu.org/emacs/nongnu.git scratch/typescript-mode or git remote add -ft scratch/typescript-mode nongnu git://git.sv.gnu.org/emacs/nongnu.git

[josteink]

Right. That merged cleanly.

Based on that, I've created a PR to ensure all the usual testing and verification is done prior to merging. And it seems some of the tests which used to pass are now failing.

You can see the build results here: https://github.com/emacs-typescript/typescript.el/actions/runs/12835889011/job/35796344055?pr=193

Some tests are probably expected to fail (due to intentionally altered behaviour), but some others may not be. I think as the author of the patch you are probably best suited to make that decision.

Now trying to run the tests locally (make test), I see that can easily fail if you don't have a particular esoteric tool (eask installed), so I can't blame you for not having been able to run and verify that prior to sending the patch.

So to make that easier, I've merged a quick fix which should allow make test to run without any other dependencies into git master: 8818e76

Could you update your nongnu-branch, and see if you can sort the tests out?

[monnier]

Will do.

[monnier]

Based on that, I've created a PR to ensure all the usual testing and verification is done prior to merging. And it seems some of the tests which used to pass are now failing.

One half was a silly error in my patch, the other half is because as you expected it's not quite that trivial to distinguish method definitions from method calls.

Could you update your nongnu-branch, and see if you can sort the tests out?

I updated it (and split the commit into two).

[josteink]

Looks good to me. Isolating the function-call vs function-definition as its own method seems like a reasonable approach.

Thanks for the contribution. Consider it merged!