-
-
Notifications
You must be signed in to change notification settings - Fork 140
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
[core] Emit an Event whenever a Grammar is AutoAssigned #907
base: master
Are you sure you want to change the base?
Conversation
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Seems fine to me!
We can "wait until there's a user for it", to follow the example of the Linux kernel project's conventions? Or merge it now ahead of its first user. Either way, I guess.
Some questions on this: First, is that I'm unsure when this event is going to be emitted. Suppose I open a Clojure file - will this event be emitted? And what if I open a Second is - usually, we treat event emitters as implementation details. Care to add an API |
@mauricioszabo This event would only ever be triggered when a buffer is auto-assigned a Grammar of "Plain Text". So the answer to Clojure would be probably not, as Pulsar has built in support. But if you open an Although documentation is a very good idea to add here, and I'll happily look into adding a method that can be called as well here. |
Oh, I see. Is it possible to be triggered with some different grammar? Do we even auto-assign some grammar if we have a plug-in for it? |
@mauricioszabo To my knowledge and testing this would only ever be triggered when being auto assigned to "Plain Text" as a fallback. Meaning that if "Plain Text" is the right grammar, such as opening a |
If anything, a tweak to the PR title might clear up the use-case for this. EDIT: The actual event name and code comment might be clarified as well. I think "fallback" has about the right ring to it, or "default." Something to imply this is a placeholder decision when the right choice wasn't as clear or explicit as it sometimes is. EDIT 2:
Isn't this, like, resetting if the user had manually picked a specific language before we call this? So if it's a And if there was nothing manually set, I would still expect the event to fire regardless of if the language for the TextBuffer was changed by calling this function? Maybe good to include what language was in it before in the event output, IDK, it depends on the use-case really. As such I think the name and code comment in the PR is fine but would want to make sure this does what you need it to do @confused-Techie ? (If it does exactly what you describe, then I think that means the code comment and the naming of the function, existing from before this PR, are a bit misleading.) |
@DeeDeeG In my testing this function does exactly as I described. But we can wait on this one for further testing to ensure that if we would like, since those tests were done a while ago and things are a bit foggy. But also the purpose of this PR is to interact with #909 so in my mind doesn't really have much urgency to be merged until I finish the feedback on that PR, which I've been lacking on doing so |
This PR adds a new event that's emitted whenever the
GrammarRegistry
auto assigns a grammar to a file.The event will contain the payload of:
This change is required for the feature I'm attempting to add to core that would allow auto-finding of packages to support unsupported file extensions.
But this event can be used to emit always, so that if there is some other potential use for this information other community packages can take advantage.