Definitions don't stand out #192
Comments
|
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? |
|
Yes, it's about |
|
Right. In that case, I'll have to confess I've stopped actively maintaining 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) |
|
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
|
|
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! |
|
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.
|
|
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 😄 |
|
````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
|
|
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 ( So to make that easier, I've merged a quick fix which should allow Could you update your nongnu-branch, and see if you can sort the tests out? |
|
Will do.
|
|
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).
|
|
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! |
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-facewould be a good choice.The text was updated successfully, but these errors were encountered: