[grammar-finder] Add new Core Package #909

confused-Techie · 2024-02-02T10:01:44Z

Requires #907

This Draft PR adds a new core package grammar-finder to Pulsar.

The decision to add this package is still under review, and until such time as it's agreed on, this PR will remain in draft status.

Grammar-Finder is a package that takes advantage of the new backend feature that allows searching of packages via the file extension that the grammar within the package supports.

This package comes with two ways to discover these packages:

By manually triggering the Command Palette grammar-finder:find-grammars-for-file which will grab the extension of the current open file and search for any packages on the PPR that support said extension.
AutoFind this feature is really the selling point of the package. It uses the new event introduced in [core] Emit an Event whenever a Grammar is AutoAssigned #907 and if Pulsar is unable to find a suitable grammar for a file, opening the default Null Grammar, it'll automatically search for a community package that supports this file extension and create a notification for the user to view the list.

In implementing this package I tried to put as much room as possible for removing interactions the user doesn't want. Such as being able to use the command palette even if AutoFind is disabled to find a suitable package.

Additionally, this package has a config that allows an array of file extensions that can be added , which it will ignore for any and all checks and never prompt for installable packages.

… too

savetheclocktower

Some suggestions for the prose. I’ll make broader suggestions in a follow-up comment.

packages/grammar-finder/README.md

packages/grammar-finder/lib/main.js

packages/grammar-finder/package.json

packages/grammar-finder/README.md

savetheclocktower · 2024-02-03T08:39:17Z

A couple things:

I share the feeling expressed elsewhere that viewing a simple list of possible grammars to install might not give the user enough context. If I knew nothing about how you'd implemented this, and I just heard the feature described, I'd envision it linking to a search results page on web.pulsar-edit.org.

There are certainly pros and cons to that approach, and keeping it all within the editor makes it a more streamlined experience. But suppose I open a tex file and am told that Pulsar can install a grammar for it. Great! Which one do I pick? If I had to pick quickly, probably the one with more downloads — but the top two candidates are pretty close to one another in that respect, and I'd probably want to dig into each one and see which has more features, which was more recently updated, et cetera.

I don't want to make an ultimatum here because I'm certainly not the intended user of this feature. Just mentioning it in case a lot of other people have the same feeling.
Let's make sure that, when tests are added, they cover these edge cases. I'm not saying I know exactly what the package should do in each of these situations, but let's at least brainstorm about it ahead of time:
- What happens when the file type actually warrants a plain text grammar? Like a txt file, or a README with no file extension? Those two might be easy to guard against, but I'll bet there are plenty more. At the very least, those two file extensions should be listed in ignoreExtList by default.
- This won't appear when the user creates a new untitled document, right? I gather that the lack of a file type would preclude the notification?
- What happens when the user already has a package that can understand the given file type, but that package is disabled? Do we try to detect that scenario?
- What happens if the user hits Esc or clicks the close button? As in “I’m not choosing any of these options explicitly; I’m just busy and would prefer to think about it later”? (I think we can prevent the notification from being dismissed in these manners, but should we?) Do we take that to mean “ask again literally the next time they open this kind of file”? Or do we try to set a flag not to ask them again… when? For 24 hours? Until the window is reloaded?

confused-Techie · 2024-02-03T09:26:20Z

@savetheclocktower I appreciate the extremely well thought out review and questions here, and I'll go ahead and address what I can think of right away, but otherwise will likely need to think a bit on a few of these.

For your questions on behaviors I want to quickly draw the distinction between the Command Palette invocation of this package and the AutoFind feature. When this package is triggered via the Command Palette it will always show community packages unless it finds none, or that extension is listed as an ignored extension. But for the rest of my answers I'll say them strictly from the perspective of the AutoFind feature.

When `text.plain` is the best grammar

This package checks strictly for text.plain.null-grammar. Which as far as I can tell is only used when Pulsar can't find the right grammar.

For example when I open test.txt the grammar scope name chosen is text.plain. So this AutoFind wouldn't do anything, as a proper grammar was selected.

So as long as the actually plain text grammar is being used, then this package won't act on it. Of course there may be a situation where plain text is the best and Pulsar also happens to be falling back to a null grammar, but unless we can come up with a common list of extensions where this happens, that may be somewhat impossible to protect against. And somewhat against the purpose of the AutoFind feature, which is intended to only act when Pulsar can't find a grammar.

But to continue, your example of a file named README is a good one. Since Pulsar falls back to text.plain.null-grammar. Which makes does cause this package to act. But in this case, since there's no community packages that offer support of this extension (Which is detected as README), nothing happens. Since I did really want to make this package as unobtrusive as possible for any automatic features. If the package errors out (in a known way) or otherwise can't find any packages, it just does nothing. Not bothering the user at all.

So the second part of the answer to when text is really the best, I'd like to think that is a self solving problem. If text is truly the best, no community packages will exist to support it, so that this package will end up failing to find any suitable community packages and end up doing nothing. Sure that all comes at the cost of a few CPU cycles wasted, but is nice in theory.

Blank New Document

You are right, in this case no extension can be identified, so it's unable to find any packages at all. Simple staying silent and doing nothing.

If a suitable package is currently disabled

As of now we do absolutely nothing to detect this situation. Since the only sources of information for this package come from:

The current buffer's file extension
The current scope of the selected grammar
The API results from the backend

Nothing beyond that is currently considered, but if we thought it important to do nothing in this situation we could certainly look at implementing it. Since I'm all for making this package as unobtrusive as possible.

Hitting `Esc`

If esc is hit while viewing the list of packages, only that modal goes away. Leaving the notification up until it's closed or esc is hit again. If the notification is closed we currently do nothing to prevent asking again.

But adding this behavior is honestly a really good idea. I'd personally say we cache extension dismissals until the current window is closed. We could just store that as a local variable of the package, and let it get destroyed naturally. Then of course if the user wants a more non-temporal way to dismiss that extension, they could always add it to the extension ignore list.

But for the concerns of making informed decisions about the best package, that seems to be the most obvious thing lacking so far, and will absolutely take some time to figure out how best to address it. Whether that means making that information available in this view, or passing off the actual request to settings-view remains to be seen.

Again thanks for taking the time to look at this one

Co-authored-by: Andrew Dupont <[email protected]>

packages/grammar-finder/README.md

Co-Authored-By: Andrew Dupont <[email protected]>

confused-Techie · 2024-02-03T09:43:53Z

Alright, added your suggestions as well as made one small change:

I like what you said about trying not to prompt twice, so I've added a very simple implementation for this.

In a local non-cached variable we track every extension AutoFind prompts for. Then we don't prompt if the extension is on that list.

So that in the event the user opens an unrecognized file then closes it and opens again, they won't get two of the same notification.

Or if they open multiple of the same kind of file they won't get a prompt for every single one.

Hopefully this can cull the lowest hanging fruit to this problem

packages/grammar-finder/README.md

Daeraxa · 2024-02-03T23:39:23Z

On the topic of "known plaintext files" how does/should it behave with files that have no extension - common for Linux config files.

There is also one format I can imagine could both be annoying and useful at the same time which would be for .log. The chance of there being a correct grammar for your specific .log file is rather low but there are already three packages out there that do supply for that extension - just not anything I personally would have much use for.

Co-authored-by: Daeraxa <[email protected]>

confused-Techie · 2024-02-04T02:07:28Z

@Daeraxa For files that have no extension, it will end up looking at the name of the file for the extension.

This is because of how NodeJS's path.parse() method behaves, I slightly disagree with it.

Take for example the following:

.nvm => { name: "nvm", ext: "" }

Here I'd personally say that the extension is nvm and the file has no actual name, but NodeJS disagrees. But even more existing packages think the same way here, for example the package that provides syntax highlighting for GitIgnore files declares the extension as gitignore. So if we don't get an extension from path.parse() we fallback to using the name just in case this is happening.

Which will also mean for files that legitimately have no extension like README the name will be used instead. Which if memory serves will likely be the same way it's handled within Pulsar itself, so hopefully that stays consistent.

As for any specific examples like log that could arguably be a default on the ignored extension list, since usually they are rather specific in terms of a usable grammar, but it's hard to say without taking a deeper look at what currently exists.

mauricioszabo · 2024-04-30T14:40:10Z

packages/grammar-finder/lib/main.js

+    // session storage. Where the next time the editor is opened it's info is gone
+    this.promptedForExt = [];
+
+    atom.grammars.emitter.on("did-auto-assign-grammar", async (data) => {


This relates to my comment: #907 (comment)

Basically, we usually have an API like onDid... to add callbacks. I think it's a good idea to add one to the other PR, and use it here.

In any way - we should never add a callback without a cleanup, so we might want to add a CompositeDisposable here and dispose it when deactivating package

mauricioszabo · 2024-04-30T14:41:22Z

packages/grammar-finder/lib/main.js

+        return;
+      }
+
+      const packages = await this.checkForGrammars(ext);


I don't think you can use await here, right? We need activate to be async, or move this to a different method...

mauricioszabo · 2024-04-30T14:42:23Z

packages/grammar-finder/lib/main.js

+      atom.commands.add("atom-workspace", {
+        "grammar-finder:find-grammars-for-file": async () => {
+          // Here we can let users find a grammar for the current file, even if
+          // it's already correctly identified
+          const grammar = atom.workspace.getActiveTextEditor().getGrammar();
+          const buffer = atom.workspace.getActiveTextEditor().buffer;
+
+          let extOrFalse = this.inspectAssignment(
+            {
+              grammar: grammar,
+              buffer: buffer
+            },
+            {
+              ignoreScope: true
+            }
+          );
+
+          if (!extOrFalse) {
+            // We didn't find any grammar, since this is manually invoked we may want to alert
+            atom.notifications.addInfo("Grammar-Finder was unable to identify the file.", { dismissable: true });
+            return;
+          }
+
+          let ext = extOrFalse.replace(".", "");
+
+          const ignoreExtList = atom.config.get("grammar-finder.ignoreExtList");
+
+          if (ignoreExtList.includes(ext)) {
+            // we have been told to ignore this ext, since manually invoked we may want to alert
+            atom.notifications.addInfo("This file is present on Grammar-Finder's ignore list.", { dismissable: true });
+            return;
+          }
+
+          const packages = await this.checkForGrammars(ext);
+
+          if (packages.length === 0) {
+            // No packages were found that support this grammar
+            // since manuall invoked we may want to notify
+            atom.notifications.addInfo(`Unable to locate any Grammars for '${ext}'.`, { dismissable: true });
+            return;
+          }
+
+          // Lets notify the user about the found packages
+          this.notify(packages, ext, `'${packages.length}' Installable Grammars are available for '${ext}'.`);
+        }


Can we move all of this to a method, so it's easier to read?

mauricioszabo · 2024-04-30T14:43:22Z

packages/grammar-finder/lib/main.js

+  }
+
+  inspectAssignment(data, opts = {}) {
+    console.log(`grammar-finder.inspectAssignment(${data.grammar.scopeName}, ${data.buffer.getPath()})`);


Can we only log on dev mode? Seems like a "debug info" here.

mauricioszabo · 2024-04-30T14:45:13Z

packages/grammar-finder/lib/main.js

+        ext = parsed.name;
+      }
+
+      console.log(`File: ${filePath} - Ext: ${ext}`);


Same here about a dev/debug mode

mauricioszabo · 2024-04-30T14:45:46Z

packages/grammar-finder/lib/main.js

+  }
+
+  async checkForGrammars(ext) {
+    this.superagent ??= require("superagent");


What is superagent?

mauricioszabo · 2024-04-30T14:48:16Z

packages/grammar-finder/styles/grammar-finder.less

+
+@package-card-background-color: lighten(@tool-panel-background-color, 8%);
+
+.list-group .package-card.selected::before {


Because of the way CSS works on Pulsar, it's always a good idea to make the "core" selector the same as the package name. So, instead of .package-card, use something like .grammar-finder, or even grammar-finder.package-card, WDYT?

confused-Techie added 2 commits February 1, 2024 20:21

Add an event whenever a grammar is auto assigned that can be listened…

fddc8c4

… too

Add grammar-finder

818a74c

savetheclocktower reviewed Feb 3, 2024

View reviewed changes

confused-Techie and others added 6 commits February 3, 2024 01:27

Update packages/grammar-finder/README.md

d49e0bd

Co-authored-by: Andrew Dupont <[email protected]>

Update packages/grammar-finder/README.md

687133f

Co-authored-by: Andrew Dupont <[email protected]>

Update packages/grammar-finder/README.md

f227fa6

Co-authored-by: Andrew Dupont <[email protected]>

Update packages/grammar-finder/README.md

e7f26ca

Co-authored-by: Andrew Dupont <[email protected]>

Update packages/grammar-finder/package.json

1224515

Co-authored-by: Andrew Dupont <[email protected]>

Update packages/grammar-finder/README.md

7e54ac2

Co-authored-by: Andrew Dupont <[email protected]>

confused-Techie commented Feb 3, 2024

View reviewed changes

packages/grammar-finder/README.md Outdated Show resolved Hide resolved

confused-Techie and others added 3 commits February 3, 2024 01:29

Update packages/grammar-finder/README.md

d695ef7

Clarify notification action prompts

3a6c76b

Co-Authored-By: Andrew Dupont <[email protected]>

Track what extensions we have prompted for per editing session

ac09e4f

Daeraxa reviewed Feb 3, 2024

View reviewed changes

packages/grammar-finder/README.md Outdated Show resolved Hide resolved

Update packages/grammar-finder/README.md

e746b95

Co-authored-by: Daeraxa <[email protected]>

confused-Techie added 2 commits February 3, 2024 19:34

Include badges, downloads, and stars in package preview

4a998f5

Use fetch and remove superagent

3b526a9

confused-Techie mentioned this pull request Feb 15, 2024

[core] Emit an Event whenever a Grammar is AutoAssigned #907

Open

mauricioszabo requested changes Apr 30, 2024

View reviewed changes

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

[grammar-finder] Add new Core Package #909

[grammar-finder] Add new Core Package #909

confused-Techie commented Feb 2, 2024

savetheclocktower left a comment

savetheclocktower commented Feb 3, 2024

confused-Techie commented Feb 3, 2024

confused-Techie commented Feb 3, 2024

Daeraxa commented Feb 3, 2024

confused-Techie commented Feb 4, 2024

mauricioszabo Apr 30, 2024

mauricioszabo Apr 30, 2024

mauricioszabo Apr 30, 2024

mauricioszabo Apr 30, 2024

mauricioszabo Apr 30, 2024

mauricioszabo Apr 30, 2024

mauricioszabo Apr 30, 2024


		@package-card-background-color: lighten(@tool-panel-background-color, 8%);

		.list-group .package-card.selected::before {

[grammar-finder] Add new Core Package #909

Are you sure you want to change the base?

[grammar-finder] Add new Core Package #909

Conversation

confused-Techie commented Feb 2, 2024

savetheclocktower left a comment

Choose a reason for hiding this comment

savetheclocktower commented Feb 3, 2024

confused-Techie commented Feb 3, 2024

When text.plain is the best grammar

Blank New Document

If a suitable package is currently disabled

Hitting Esc

confused-Techie commented Feb 3, 2024

Daeraxa commented Feb 3, 2024

confused-Techie commented Feb 4, 2024

mauricioszabo Apr 30, 2024

Choose a reason for hiding this comment

mauricioszabo Apr 30, 2024

Choose a reason for hiding this comment

mauricioszabo Apr 30, 2024

Choose a reason for hiding this comment

mauricioszabo Apr 30, 2024

Choose a reason for hiding this comment

mauricioszabo Apr 30, 2024

Choose a reason for hiding this comment

mauricioszabo Apr 30, 2024

Choose a reason for hiding this comment

mauricioszabo Apr 30, 2024

Choose a reason for hiding this comment

When `text.plain` is the best grammar

Hitting `Esc`