-
Notifications
You must be signed in to change notification settings - Fork 79
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
Add typescript faces #78
Conversation
Hey @tam5 and thanks for the patch! Contributions are very much welcome, so sorry about the slow response. I've been on vacation, and didn't really have much spare time until now. From what I can tell you've updated the tests and added documentation. That's great and I really like that! What I'm not sure about is the implementation of the change itself. From what I can see, you're just rebinding all the standard Emacs-faces into buffer-local variables, and overriding their value. While that might work, it's a kind of "hack". Wouldn't it be better to update the Is there any particular reason you chose to use the approach you did? Is this conventional in other major-modes? I ask because I honestly don't know myself. [1] https://github.com/ananthakumaran/typescript.el/blob/master/typescript-mode.el#L289-L305 |
@josteink thanks for the review! The short answer is I chose this approach because it made the smallest impact on the existing code. It seems like a rather minimal effort to take the other approach, so I can switch to that if you think thats better, but first I'll give the longer answer: First of all, I am fairly new to elisp so take what I say with a grain of salt. In #32 I did see that you offered the suggested implementation you are speaking of, and I admit that was my initial path as well. However, though the current implementation may seem a bit like a hack, I think it may actually provide a better separation of concerns. This approach is taken by php-mode, and though I don't know for sure the rational, my assumption is as follows: PHP mode uses the cc-mode engine, which provides some utilities for setting the fontification, and therefore needs this hack, as php-mode alone is not directly responsible for defining all of the font-lock faces in php. From what I've seen so far, it seems fairly uncommon to add specific overides for font-lock as we are proposing to begin with, and most major modes are only adding faces for things font-lock doesn't cover. (so automatically, no, this is not conventional) Perhaps I'm not being very clear, but what this all points to is the fact that a major mode should ideally be using font-lock when it can. The fact that we want to add a bit more control on top of that seems like a separate feature, and thus can be implemented separately. Furthermore, I think this implementation allows for greater flexibilty when it comes to future changes. |
@wasamasa: You seem to know all things about Emacs and pitfalls associated with various ways of solving different kind of Emacsy problems. Do you have any opinion on this kind of solution? Does it look OK to you? |
First of all, why? What is the rationale of this? All introduced faces are named the same as the original faces. The only reason I can think of is that you're dead-set on emulating a certain editor/IDE color scheme and have discovered that Emacs has a set of generic faces that are used for all languages, with no way to change the colors per language. Solving this per mode is just wrong (it doesn't scale at all, there are too many programming languages and modes for them out there, some of which are part of Emacs), better use/write a package that allows you to override faces per mode/buffer. I'm sure they do exist. |
Thanks @wasamasa! That was exactly the kind of out-of-the-box thinking I was hoping for. @tam5: If there's already a generalized solution to the problem, I fail to see why we should invest in creating a specialized implementation for our own major-mode. As such, I'm now quite in favour of closing this PR unless convinced otherwise. Besides closing an open issue (which I guess we can close anyhow as "wont fix"), are there any other good reasons you see for implementing this? |
Indeed, that makes sense. That’s kinda what I meant by
I’m ok with closing this as won’t fix. The main motivation I would see in implementing this would be more for organizational purposes. If Oh, and as for emulating the color theme of other ide’s, what’s wrong with that? Of course I want to do that |
At first I thought it was odd that seemingly no default One I did find having it though was To be quite honest, I'd rather be more happy to have an official As for #39, the only other thing I can see which is non-standard is the decorator thing. Technically speaking that's a function (or constructor?) though... So I'd be happy to use
If so... I'm inclined to do so. Thanks for putting in an effort and trying to improve on this mode. It's really appreciated! Sometimes though even the best of PRs ends up closed for a variety of reasons. Please don't be discouraged from working on #39, even though we closed this one. |
closes #32