-
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
fix(cspell): sort and unique ignore lists #36009
Conversation
This pull request has merge conflicts that must be resolved before it can be merged. |
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.
Can you make this a CI check too? To avoid people bypassing it or committing from web interface.
3d7b2ec
to
670a938
Compare
Let's not prevent people from contributing. Like other stuff, we can always auto-fix it at midnight. |
Most people do not change cspell files. Those that do (usually to address the weekly spellcheck errors) should ensure that they are changed in a correct manner. Our Prettier review bot is the best contribution model. No bad content goes into the master branch and there's minimal friction to both contributors and reviewers. I think it should be implemented for everything, although Markdownlint is slightly different because the effect is non-local: a redirect can cause fixes in many files that you didn't touch. I would still advocate for local fixable changes (i.e. errors caused by changed lines) to be reported by the bot, though. And surely people change the dictionary file about once a week, so we should do everything properly in one commit. |
We mark the issues good first issues. So we should expect new contributors fixing typos. How do you propose to handle this?
|
Unfortunately I would expect them to fix obvious typos but not, say, add things to dictionaries. It's too much of a judgment call to tell what to do with each word and I'm afraid, putting myself into the shoes of a new contributor, they would not have the courage to do. For example, who knows if I should add Ideally it would be reviewdog. But the changes are too non-local and result in proposed diff located in uncommentable areas. So, the workflow could output a whole dictionary file and ask them to copy&paste, or it could output a diff that we can apply (since ideally it would only be a few lines added each time anyway). What I don't want is two PRs every week, one called "fix typos" another called "fix dictionary file", both touching the same file. Do it once and do it right. |
Agree. Fixing typos in code is not good first worthy. I'll remove the good first label. It's not possible to comment on unedited lines. I'll create a new check workflow that will run when the list files are edited. |
This pull request has merge conflicts that must be resolved before it can be merged. |
06d6a68
to
f6827ca
Compare
a = a.toUpperCase(); | ||
b = b.toUpperCase(); | ||
return a < b ? -1 : a > b ? 1 : 0; |
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.
Use Intl.Collator
with sensitivity: "accent"
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.
The collator sounds good but cSpell doesn't seem use collator for comparisons. So after formatting the files using collator, I am getting the following errors from cspell because now the code treets 'acosh' = '𝚊𝚌𝚘𝚜𝚑'.
files/en-us/web/javascript/reference/global_objects/intl/displaynames/of/index.md:28:153 - Unknown word (nonterminal)
files/en-us/web/javascript/reference/global_objects/intl/displaynames/of/index.md:68:48 - Unknown word (canadien)
files/en-us/web/javascript/reference/global_objects/math/acosh/index.md:14:300 - Unknown word (𝚊𝚌𝚘𝚜𝚑)
files/en-us/web/javascript/reference/global_objects/math/asinh/index.md:14:204 - Unknown word (𝚊𝚜𝚒𝚗𝚑)
files/en-us/web/javascript/reference/global_objects/math/atanh/index.md:14:397 - Unknown word (𝚊𝚝𝚊𝚗𝚑)
files/en-us/web/javascript/reference/global_objects/math/cbrt/index.md:14:83 - Unknown word (𝚌𝚋𝚛𝚝)
files/en-us/web/javascript/reference/global_objects/math/expm1/index.md:14:83 - Unknown word (𝚎𝚡𝚙𝚖)
files/en-us/web/javascript/reference/global_objects/math/hypot/index.md:14:109 - Unknown word (𝚑𝚢𝚙𝚘𝚝)
files/en-us/web/javascript/reference/global_objects/math/log/index.md:14:182 - Unknown word (𝚕𝚘𝚐)
files/en-us/web/javascript/reference/global_objects/math/log10/index.md:14:182 - Unknown word (𝚕𝚘𝚐)
files/en-us/web/javascript/reference/global_objects/math/log1p/index.md:14:192 - Unknown word (𝚕𝚘𝚐)
files/en-us/web/javascript/reference/global_objects/math/log2/index.md:14:182 - Unknown word (𝚕𝚘𝚐)
files/en-us/web/javascript/reference/global_objects/math/sqrt/index.md:14:179 - Unknown word (𝚜𝚚𝚛𝚝)
The MathML pages use non-ASCII characters(a-zA-Z), that look like ASCII, in the formulas. I don't know the reason for using non ASCII abcd characters in the MathML code. Need to ask @fred-wang. Should we change those to make ASCII?
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.
The MathML formulae often use the monospace Latin characters. Including JS pages I wrote, because I converted them using TeXZilla, so yes I'd be curious to hear what Frédéric says, including whether it's possible to make TeXZilla use inline CSS instead.
I'm surprised that 𝚌𝚋𝚛𝚝
and cbrt
are considered equal though. Maybe we should keep using toLowerCase
then—just change the regex to use a.toLowerCase() === b.toLowerCase()
.
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.
Now I've used toLowerCase
for all the comparisons.
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.
👍
PR #36109 tested this PR check workflow.
|
* fix(cspell): sort and uniq ignore lists * use toLowerCase for comparisons
The PR adds pre-commit hooks to sort and uniq the cSpell ignore list files.