-
Notifications
You must be signed in to change notification settings - Fork 22.5k
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 a lint rule for e-mail #23146
Add a lint rule for e-mail #23146
Conversation
@Josh-Cena the solution depends on how far we are going to go with spellings: For option #a we can keep adding words like this PR does. For option #b we implement dedicated replacement list rule in the linter:
For option #c we can use tools like > cspell --show-suggestions index.md
.../index.md:2:8 - Unknown word (heiallo) Suggestions: [hello, hallo, niello, heirloom, heimdall]
.../index.md:13:1 - Forbidden word (e-mail) Suggestions: [email, emails, entail, emil, camail]
CSpell: Files checked: 1, Issues found: 2 in 1 files I think for the long-run we can use cSpell, as cspell will integrate well with VSCode as well because it's port of VSCode plugin "Code Spell Checker". |
I know @nschonni is already running cSpell locally, and I also have a copy of his config for my own VS Code config, so if he's taking care of other incorrect spellings anyway, this should be handleable as well. |
Thanks @OnkarRuikar. I agree with Josh that this is a lot of work per disallowed word and we should look at alternatives. Cspell might be a good avenue, I'm also using it in VSCode and adding words to a user config as I go, but it's still going to need some tweaks. If you run it over cspell -c ~/mdncspell.json --unique --no-progress files/en-us/web/javascript/**/*.md
...
./web/javascript/reference/statements/do...while/index.md:19:47 - Unknown word (dowhile)
./web/javascript/reference/statements/export/index.md:270:57 - Unknown word (htmlelement)
./web/javascript/reference/statements/for-await...of/index.md:21:47 - Unknown word (forawaitof)
./web/javascript/reference/statements/for...in/index.md:16:47 - Unknown word (forin)
./web/javascript/reference/statements/for...of/index.md:19:47 - Unknown word (forof)
./web/javascript/reference/statements/function_star_/index.md:290:13 - Unknown word (Lindesay)
./web/javascript/reference/statements/import/index.md:229:3 - Unknown word (Limin)
./web/javascript/reference/statements/import/index.md:229:20 - Unknown word (Terlson)
./web/javascript/reference/statements/let/index.md:98:5 - Unknown word (Redeclarations)
./web/javascript/reference/statements/throw/index.md:134:7 - Unknown word (ZIPCODE)
./web/javascript/reference/statements/throw/index.md:158:17 - Unknown word (rethrown)
./web/javascript/reference/statements/try...catch/index.md:17:47 - Unknown word (trycatch)
./web/javascript/reference/statements/try...catch/index.md:100:3 - Unknown word (myroutine)
./web/javascript/reference/statements/var/index.md:23:5 - Unknown word (varname)
./web/javascript/reference/strict_mode/index.md:120:39 - Unknown word (Varible)
./web/javascript/reference/strict_mode/index.md:164:109 - Unknown word (undeletable)
./web/javascript/reference/template_literals/index.md:126:12 - Unknown word (collapser)
./web/javascript/reference/template_literals/index.md:331:72 - Unknown word (DSLs)
CSpell: Files checked: 904, Issues found: 1964 in 471 files Approx 2000 unknown words under JS docs, so that rules out option a) at least for all types of typos (including uncommon words). I like the links you have pointed to for word lists, these would be useful to include in a spellchecker for common typos (style enforcement). I think the markdownlint job should just check formatting and we can tackle spelling in a separate effort that's easier to maintain, what do you think? |
From the conversation in
I agree with the argument because we'll have to add a lot of words to our custom allow list like: abortable, componentized, accentcolortext, appcache, redirectors, authorid, mergeable etc.
Even if we add these to our ignore list and setup a spellchecker, we'll have to keep updating the list for new contributions or ask PR authors to correct them. I ran cspell on the entire repo today it flagged ~5779 spelling errors.
Middle way is to maintain a bad word list (like this) only for words which we don't want to promote e.g. e-mail, web site. I am working on option b. It'll add ability to specify arrays in the rule config in
Difference between handling it in linter vs spellchecker would be ability to auto fix. The linter will be able to autofix If we want to to add a spellchecker then bigger discussion is needed. May be on https://github.com/mdn/mdn-community/discussions? |
All, I've implemented and added a new generic rule for handling the incorrect spellings. Now the config will like this:
To add to above discussions, using linter we can ban split words like /cc @nschonni. |
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.
Small suggestions that might not be possible. Thanks, @OnkarRuikar
Thank @OnkarRuikar. I was initially thinking it should be "either or" with the markdownlint job versus a dedicated spellchecker, but maybe we could make use of both. It looks like checking in a cspell config at some point might be a good idea regardless of how it's enforced (I spotted you can include a lorem-ipsum dictionary, too, see this gist). Warnings via spellcheck on changed files in PRs combined with adding flagged words to the config as we go might help.
Yeah this is useful.
I agree with this. I think this PR is a useful change for the meantime 👍🏻 Some notes / things to look at alongside this PR or for a separate spellchecker discussion: |
One more point to add to the discussion, what to do with incorrect spellings coming from external resources? a. In URLs:
[Colour Manegement Guide](www.blogs.com/colour-manegement-guide)
b. Article titles:
For more info refer the article "Using CSS Property appearence" on www.abc.com.
c. In external code snippets:
Consider following code snippet taken from the example ( www.github.com/mdn/interactive-examples/abcd.html)
```js
let colar = 'green';
``` Should we correct it on our side? Or Do we add a setting/magic comment to ignore the spot? |
034ce39
to
689cc8b
Compare
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.
Config change LGTM.
0968df1
to
9492f86
Compare
d3303b0
to
bbe87e2
Compare
Co-authored-by: Schalk Neethling <[email protected]>
It looks like the |
Seems like GitHub picked up the detached commit ca3b60c as base for rebase, then workflow started working on it then then GitHub course corrected itself. Will putting some delay in workflow before fetching changed files solve the issue? It'll be hard to reproduce and test though. :( |
* Add a generic bad-spelling rule * Apply suggestions from code review Co-authored-by: Schalk Neethling <[email protected]> --------- Co-authored-by: Schalk Neethling <[email protected]>
With this the linter will not allow
e-mail
.