-
Notifications
You must be signed in to change notification settings - Fork 80
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
Enable more colors in typescript #79
Comments
All those changes sounds good to me, and if someone wants to put in the work to implement it, I will be happy to review and merge. Right now we have 3 font-lock levels. If we're adding these new ones... Maybe that should go in a fourth? If so, I'd support making the fourth one the new default, but I know there are people who in the past have thought that to be a bit "too busy", so they should have the option to keep things as they are (that is, customize to stay on level 3 or below). Does that sound good? |
Putting the additional highlighting in a 4th level seems reasonable. Though I wonder in practice whether it may make integration with other tools (minor modes, etc.) more complicated. I have no evidence one way or the other. There's probably a segment of our user-base that would welcome the added fontification, but someone who wants to see this functionality integrated to I'm not in favor of marking function fnord(window: number): void {
console.log(window); // `window` is not a builtin here
}
fnord(1); (Case in point, Github's fontification erroneously marks In workers neither There's also the suggestion that import { Foo } from 'bar' Surely At first glance, the other cases appear unproblematic but I may have missed something. |
@josteink & @lddubeau, I am happy to put some time into this. I'll just need some review comments when done, I assume particularly related to how best to integrate with what already exists. To the point of customization, font-lock levels, and "too busy": I would argue (only very slightly) against adding a new font-lock level and instead including this as level 3, for simplicity. I have to assume most people would welcome the added fontifications, as they do exist pretty ubiquitously in VsCode, Sublime, etc. Additionally, for people who like "less business", I can only assume that they are generally going to be using a fairly dulled out color theme anyway. Lastly, after pr #78 is complete, and assuming this is implemented in a similar fashion, the end user should always have full control of how things are displayed anyway, and as long as we document it clearly enough, I don't see why that should pose any problems. @lddubeau to your reservations regarding the builtins and the imports, both very good points, and i agree, let's not include those. @lddubeau to your last point regarding fontification based on actual code analysis: yes, that would be amazing if we could ultimately get that to work right, but I assume it is a much higher effort than a few regex's for highlighting which do the trick in 99% of cases. In fact, based on your example I tested how VSCode would highlight the following: |
@tam5: That's great! Thanks! Let us know when you have something you want reviewed, or if you want our feedback on how we think things may best be done. We'll be around!
I agree here. I guess that makes a full consensus.
@lddubeau has some great points and thoughtful comments (as always). This point about how highlighting based on types, etc can only be 100% accurate if powered if we have proper a code-analysis is as true as ever. While I really respect his drive for accuracy in what this mode provides its users, I'm more lenient to pragmatic/practical which may not be entirely correct for all use-cases. Basically doing proper code-analysis with regexps alone is really, really hard (if not impossible), and that we may get some false positives here and there should not stop us from implementing something which does the right thing most of the time. Especially if the other option is no syntax-highlighting on something people commonly want syntax-highlighted. So yeah. I think somewhere in the middle-ground we should be able to do some improvements which everyone agrees represents an improvement to the status-que... (That ofcourse, while we all await that someone, somewhere writes this new and awesome tsserver based highlighting code :) So... Go ahead and do your best. We'll be happy to review your patches! |
FYI I have emailed Emacs-devel about additional font lock faces: https://lists.gnu.org/archive/html/emacs-devel/2018-08/msg00020.html |
any updates here? |
@yintothayang I had started work on this at my fork, which I have now been using for a while, and honestly have not gotten around to rounding out the edges and making sure it is ready for a PR. If you want to try it out, or check out the code and see what else needs to be done that would be great and we can potentially merge it in. If you are writing Angular, you might also want to consider ng2-mode |
I'll take a look, thanks! |
Would love to see some progress on this! typescript-mode feels monotone in comparison to the competition :( |
@tam5: Any change your fork is getting ready for a PR? :) |
So far, as a fourth level of highlighting, I have implemented the following (see the first post for more detailed examples):
The most important thing that is still left is type hints in general, some examples: class Foo {
private bar: Bar;
^^^
constructor(bar: Bar) {
^^^
this.bar = bar;
}
} The problem I have been having with highlighting these type hints is distinguishing them from simple For what I have so far, I'm happy to open a PR and we can start to discuss some of the choices I've made etc. |
There's still some controversy/discussing about HOW such things should best be implemented, but I think having working code is a good start. If you can create a PR, I won't guarantee that we will merge it right away, but I'm definitely eager to see what the code looks like, not to mention how it ends up looking to an end-user. So yes, please create a PR and lets see where we can take this 😄 |
Closed and fixed by @tam5 :) |
Follow up to #32 which requests customizable faces specific to typescript mode.
This is addressed by PR #78, and allows the currently highlighted faces by typescript mode to be customized.
However, it does not address adding additional highlighting rules. In fact, the example given by @it6 in #32 is for
function-call
, which is not currently given a face at all.I am proposing to add additional highlighting for the following cases, each with an example, and the face name it would be given:
typescript-function-call-face
)typescript-function-name-face
)typescript-variable-name-face
)typescript-decorator-face
)The text was updated successfully, but these errors were encountered: