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

[spell-check] Allow the user to whitelist sections of a buffer… #1147

Conversation

savetheclocktower
Copy link
Contributor

@savetheclocktower savetheclocktower commented Dec 1, 2024

…for spellchecking on a per-language basis.

Closes #1135.

The linked ticket weighs the pros and cons of various ways to implement this, so I'll let it do most of the talking.

You can specify values in spell-check.grammars that you couldn't before. Some examples:

  • source.python comment to spell-check Python files, but ignore everything except comments.
  • source.python comment, source.python string works similarly, but whitelists comments and strings within Python files.
  • source comment enables spell-checking for all source files, but only within comments.
  • text enables spell-checking for all text files: HTML (text.html.basic), plaintext (text.plain), and others.

You'll notice that this PR touches the actual logic of matching grammars and determining which misspellings are excluded, but devotes most of itself to the task of actual scope matching.

Scope matching

Given a ScopeDescriptor, you'd think that it would be pretty easy to test whether it matches a given scope selector. That's what we need in this scenario; we can get the scope descriptor of a misspelling in the buffer, and we need to compare it to a selector that was specified in the package config.

In fact, ScopeDescriptor doesn't implement any method like what I just described; near as I can tell, packages do different ad-hoc things in this scenario. It's ridiculous. The ScopeDescriptor class has a single getScopesArray method and is otherwise pretty useless.

I figured I'd take time to write some scope helpers for the spell-check package as a test run for adding new APIs to ScopeDescriptor. At the very least there ought to be a ScopeDescriptor::matches(selectorString) function. It's also surprisingly common to see logic in the codebase that (e.g.) fails to recognize that source should match source.js. Atom already compromised part of the scope contract by assuming that the order of scope segments is arbitrary — that's why you might see .js.source instead of .source.js in your config.cson. That bell can't be unrung, but it would be great not to ring any other bells needlessly.

One of those ad-hoc scope-matching strategies was being applied for the spell-check.excludedScopes setting, so I was able to eliminate some code by replacing it with this new strategy.

There's another new ability here: you don't have to specify an exact top-level scope selector in the grammars setting! You can specify source (either on its own or as part of a selector like source comment) and have it match source.js, source.python, source.ts, et cetera.

Testing

New specs have been added and are passing for me locally. Existing specs also pass for me.

You should be able to test the functionality by changing your spell-check settings; add a value like source comment in the Grammars field. Verify that spell-checking is applied only to comments.

…for spellchecking on a per-language basis.

This opens up things that aren't really possible with the `excludedScopes` setting. For instance: you can specify `source.python comment, source.python string, source.js comment`; this will enable spellchecking in Python files for both comments and strings, while enabling spellchecking in JavaScript files for _only_ comments.

Closes pulsar-edit#1135.
@savetheclocktower savetheclocktower force-pushed the spell-check-scope-granularity branch from c4ce2a5 to 3d5f9ec Compare December 1, 2024 21:42
Copy link
Member

@confused-Techie confused-Techie left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Without going too in depth since it is release day, the code and concept look solid with the testing lending significant credibility to me saying lets merge it.

The only thing I do want to add, is should we enable source comment spell checking by default? I can't really see why someone wouldn't want to spell check their comments, as it is just text. It makes total sense to me to be the default behavior

@savetheclocktower
Copy link
Contributor Author

The only thing I do want to add, is should we enable source comment spell checking by default? I can't really see why someone wouldn't want to spell check their comments, as it is just text. It makes total sense to me to be the default behavior

I'm cool with it. Just updated the PR!

@confused-Techie
Copy link
Member

@savetheclocktower Fantastic! Merging now

@confused-Techie confused-Techie merged commit b04e997 into pulsar-edit:master Dec 16, 2024
5 of 7 checks passed
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.

[spell-check] Should allow the user to more easily spell-check only part of a buffer
2 participants