Skip to content
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

Open
wants to merge 1 commit into
base: master
Choose a base branch
from

Conversation

confused-Techie
Copy link
Member

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:

{
  grammar
  buffer
}

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.

Copy link
Member

@DeeDeeG DeeDeeG left a 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.

@confused-Techie
Copy link
Member Author

@DeeDeeG Thanks for the approval! I'm happy to wait for the first user of it, which I'm assuming you know is in #909

@mauricioszabo
Copy link
Contributor

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 .ini file, will it be emitted with something like "unknown format"?

Second is - usually, we treat event emitters as implementation details. Care to add an API atom.grammars.onDidAutoAssignGrammar and add documentation to it explaining the first question I had?

@confused-Techie
Copy link
Member Author

@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 .ini file and have nothing that provides syntax highlighting for it, then yes it'd be emitted.

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.

@mauricioszabo
Copy link
Contributor

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?

@confused-Techie
Copy link
Member Author

@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 .txt document, this won't be triggered. Then if a grammar is found for your language this would never be triggered. So yeah to my knowledge it only triggers in the event of us not being able to find any appropriate grammar at all for your currently open buffer.

@DeeDeeG
Copy link
Member

DeeDeeG commented May 8, 2024

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:

// Extended: Remove any language mode override that has been set for the
// given {TextBuffer}. This will assign to the buffer the best language
// mode available.

Isn't this, like, resetting if the user had manually picked a specific language before we call this? So if it's a .rb file and the user had manually chosen JavaScript as the language... maybe this resets it to Ruby?

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.)

@confused-Techie
Copy link
Member Author

@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

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants