-
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
Reorganize custom dictionaries, better spell checking infra #36255
Conversation
Give me 2-3 days. |
You are using both ellipsis and plural from of the word. How to denote at least one file is required? |
Huh, the GH actions shell does not automatically expand glob patterns. That's inconvenient. |
This is the description format as used by commander.js |
In writing docs we need to explain following points:
I am waiting on content fixes to merge to check regex and other codes. |
You can specify shell to a task:
|
This pull request has merge conflicts that must be resolved before it can be merged. |
This pull request has merge conflicts that must be resolved before it can be merged. |
Preview URLs
External URLs (1)URL:
(comment last updated: 2024-10-31 02:20:13) |
Which docs are you referring to? The comments and descriptions in the cSpell config? |
Co-authored-by: Onkar Khadangale <[email protected]>
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.
Looking good, leaving a +1, shall we wait for Onkar to have a look or are we ready to merge?
7ac7eea
to
b60649a
Compare
Will merge once @OnkarRuikar also gives a thumbsup |
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.
A few suggestions and questions. I was waiting for related content changes to merge.
In writing docs we need to explain following points:
Which docs are you referring to? The comments and descriptions in the cSpell config?
In the spellings section in writing guidelines we could have short description of cspell and dictionaries. And can mention the command to check spellings manually:
npx cspell --no-progress --gitignore --config .vscode/cspell.json "**/*.md"
media.getusermedia | ||
media.mediasource | ||
media.peerconnection | ||
media.peerconnection.rtpsourcesapi |
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.
media.peerconnection.rtpsourcesapi | |
rtpsourcesapi |
Why put the entire fully qualified name? Adding the interface name should be sufficient, right?
There are multiple occurrences of these like dom.abortablepromise
.
One word per line will keep it simple and reduce the dictionary size significantly.
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.
These are Firefox preferences. I add the prefix to prevent abortablepromise
being a valid word on its own; I want the ignored words to be as contextualized and specific as possible because it's always easier to fix false positives than catch false negatives.
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.
I add the prefix to prevent
abortablepromise
being a valid word on its own
I see. 👍
It would be great if this feature/case is documented somewhere.
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.
I've added a sentence to cspell.json's comment:
// Note: when adding words to these lists, be as specific and contextualized
// as possible, to avoid typos being masked elsewhere. For example, all FF
// preferences should include prefixes: `dom.abortablepromise` instead of
// just `abortablepromise`, which may be missing a space in other contexts.
emodeng | ||
emptytext | ||
emsdk | ||
enable-tracejit |
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.
enable-tracejit | |
tracejit |
We don't have to put the entire term. Same for color-CBDT
etc.
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.
Again, tracejit
should not be a word unless there's evidence that we actually write it. The only valid usage is in the whole context of enable-tracejit
—other than that it should be a typo.
@@ -0,0 +1,302 @@ | |||
219ffwef9w0f |
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.
219ffwef9w0f |
Like "sessionid=\\w+",
can we add Set-Cookie: .*? qwerty=\\w+
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.
I don't want to do this because Set-Cookie
could contain legitimate words in the attribute names.
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.
Also, it makes no sense to add regex if there is only one instance.
boxbg | ||
BRAH | ||
bram.us | ||
bruce_vs_ironman |
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.
bruce_vs_ironman | |
ironman |
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.
Again ironman
is not a valid word. It's "Iron Man". It's only valid in this particular file name because I don't want to change it.
Co-authored-by: Onkar Khadangale <[email protected]>
Co-authored-by: Onkar Khadangale <[email protected]>
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.
Super, thank you 👍🏻
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.
One nit
files/en-us/mdn/writing_guidelines/writing_style_guide/index.md
Outdated
Show resolved
Hide resolved
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 PR changes look good to me. Thanks for taking on this monumental task!
Co-authored-by: Onkar Khadangale <[email protected]>
Great, let's go! |
Well done 👏🏻 |
* Reorganize custom dictionaries, better spell checking infra * Update scripts/sort_and_unique_file_lines.js * Reorg files * Updates * Apply suggestions from code review Co-authored-by: Onkar Khadangale <[email protected]> * Typo * Fix action * Fix checkout * Update scripts/sort_and_unique_file_lines.js Co-authored-by: Onkar Khadangale <[email protected]> * Update .vscode/cspell.json Co-authored-by: Onkar Khadangale <[email protected]> * Add docs * Update files/en-us/mdn/writing_guidelines/writing_style_guide/index.md Co-authored-by: Onkar Khadangale <[email protected]> --------- Co-authored-by: Onkar Khadangale <[email protected]>
This should be merged after my fleet of typo fix PRs.
This PR splits our custom dictionaries by topic, making them easier to maintain and inspect. It could also allow for more granular options in the future, such as whether each one should provide suggestions, or whether they should be case-sensitive.
@OnkarRuikar @bsmth