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

Add spell checker action WordWarden #1239

Merged
merged 3 commits into from
Oct 28, 2024

Conversation

gevhaz
Copy link
Contributor

@gevhaz gevhaz commented Oct 3, 2024

Hi I was searching for something else in the issues and noticed you guys were considering options for spellcheckers in #1189. So I thought I'd suggest my own action. It basically does pre-processing by converting to HTML and removing code and links, then runs aspell on it. It's possible to run offline by using the script https://github.com/gevhaz/Word-Warden/blob/main/spellcheck.py

My action seems to discover more spelling issues than the one in #1189, for better or for worse (it can be annoying with too many false positives). I tried either adding them to a dictionary or 'correcting' them so that the job passes. In the case of words that are arguably 'code', such as as trigEngine, I fix the issue by adding backticks, since inline code will be removed in the preprocessing.

These things can be very subjective and I'm not attached to any choice between 'fixing' a word or adding it to the dictionary.

If you think that the changes I made to the markdown files are an improvement, consider merging this PR. Otherwise feel free to close it or ask for changes.

@gevhaz gevhaz marked this pull request as draft October 3, 2024 15:48
@gevhaz gevhaz force-pushed the wordwarden-spell-action branch 11 times, most recently from f050d50 to f83120a Compare October 3, 2024 18:13
@gevhaz gevhaz marked this pull request as ready for review October 4, 2024 08:17
@L3MON4D3
Copy link
Owner

Hi :)
Sorry for taking so long to look at your PR, I was busy 😅

This seems very usable, especially that there is a cli-tool so one doesn't have to wait for the action to process 👍
One potential issue is changing the visible text of a link to a heading: it will be used to generate the link for the vim-help directly, so changing the text may break it. Can that, or only the potential for breaking, be avoided? Or is it already avoided? :D

@leiserfg WDYT?

@leiserfg
Copy link
Contributor

leiserfg commented Oct 22, 2024

This is mostly fine and mergeable but I noticed at least one link wrong:
lsp-snippets-transformations)
I think Nvim help won't like it.

@gevhaz
Copy link
Contributor Author

gevhaz commented Oct 22, 2024

Hey, thanks for checking it!

The link in DOCS.md with the visible text

`lsp-snippets-transformations`

looks like this in the generated docs:

|luasnip-`lsp-snippets-transformations`|

The backticks were added by me, and they explains why it breaks. I also agree that backticks is not the 'right' way to solve this 'spelling error'. Basically there are three other choices:

  1. Add lowercase 'lsp' to the dictionary. The drawback is that you will not discover if someone writes lsp in lowercase from then on.
  2. Uppercase LSP in the link ('LSP-snippets-transformations'). Would this work?
  3. WordWarden also supports custom sed preprocessing as a last resort. You could allow the link to stay the same by doing a s/lsp-snippets-transformations// in such a preprocessing script. It can't be done by adding that full string to the dictionary since aspell doesn't support dashes in dictionary words.

I'll also check for similar broken link cases after hearing what solution you prefer.

@leiserfg
Copy link
Contributor

Option 2 seems fine to me, let's wait for @L3MON4D3

@L3MON4D3
Copy link
Owner

L3MON4D3 commented Oct 23, 2024

Thank you for the detailed reply (and @leiserfg for taking a look ❤️)!

I agree, option 2 is fine :)
Especially because the link generated in the helptext is lowercased anyway (eg. Variables-Environment Namespaces -> luasnip-variables-environment-namespaces), so this doesn't even affect it.

@gevhaz gevhaz force-pushed the wordwarden-spell-action branch from b577429 to 91287d5 Compare October 24, 2024 14:37
@gevhaz
Copy link
Contributor Author

gevhaz commented Oct 24, 2024

Great! I've updated the PR. I also double checked that there are no other similar issues.

@gevhaz gevhaz force-pushed the wordwarden-spell-action branch from 4822a08 to 46c45ad Compare October 24, 2024 14:39
@leiserfg
Copy link
Contributor

Is there a way to exempt links (the same way it happens with inline code)? So other links don't need to be changed (like vsnip.vim).

@gevhaz
Copy link
Contributor Author

gevhaz commented Oct 24, 2024

It's possible to add a pre-processing sed script. So it would be possible to replace the pattern [.*]( with [](, which would have the effect of excluding link texts.

I think the easiest thing would be to just add vsnip and similar to the dictionary though. It's unlikely that you ever misspell something as 'vsnip' specifically in the future, so there is no harm in it. And then you will also continue to benefit from spellchecking in other cases. For example, I changed the link text in the following line:

This can also be done much cleaner, with all the benefits that come with using a loader, by using the [loader for Lua](https://github.com/L3MON4D3/LuaSnip/blob/master/DOC.md#lua)

so that it's "Lua" instead of "lua".

If you agree that that is preferrable, I can remove the backticks from link texts.

@L3MON4D3
Copy link
Owner

L3MON4D3 commented Oct 25, 2024

Yeah I'd agree, spellchecking in links ist useful, if they contain a word that is misspelled it should be corrected.

@gevhaz gevhaz force-pushed the wordwarden-spell-action branch 4 times, most recently from c69390c to 9487924 Compare October 25, 2024 08:15
@gevhaz
Copy link
Contributor Author

gevhaz commented Oct 25, 2024

I've added a fixup commit which removes backticks from repo names. If it looks good, I can squash.

@L3MON4D3
Copy link
Owner

Good choice, that seems more consistent👍
I'm happy, feel free to squash :)

@gevhaz gevhaz force-pushed the wordwarden-spell-action branch from 5c8889f to 1707586 Compare October 26, 2024 14:10
@gevhaz
Copy link
Contributor Author

gevhaz commented Oct 26, 2024

Done

@L3MON4D3
Copy link
Owner

Awesome, thank you very much for the contribution! :)

@L3MON4D3 L3MON4D3 merged commit a49203d into L3MON4D3:master Oct 28, 2024
@L3MON4D3 L3MON4D3 mentioned this pull request Oct 28, 2024
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