Problem/Motivation
It is not easy to figure out how to make a sensible set of words to change. As I thought about it I decided that it was worth trying to fix words that appear 1 time in 1 file. This set of words are most likely to be very simple fixes which will be easy to review. It also means that there are less changes to be made to remove a word and thus the size of dictionary.txt can be reduced significantly for minimal work. There are around 900 words like this. Although I just found an error in the way I generated that list and I have to do it again.
All these words are used in one file. I started with 'a' and while editing spotted some words not beginning with 'a' that are used in just one file. By my estimates word beginning with a -> d that are used once represent about 20% of the total words that are used once. Some words that are used once, such as dublincore*, are already in #3210125: Fix spelling in core.* yml files, so are excluded here.
The list of words beginning with a -> e, inclusive, that are used once in one file.
Proposed resolution
Limit this to one line easy fixes, avoid any possible controversy, or any change that would better fit with other words, such as doing chien and chiens in the same patch.
Add more starting letters but keep the < 50k for easy review.
Remove the following from dictionary.txt.
42 words
-abempty
-abiword
-aewesome
-aflopend
-aliasable
-aliquet
-allowtransparency
-apng
-asubject
-autodiscovered
-autofilling
-autoincrement
-autoincrementing
-autoindex
-baseroot
-bistromathic
-blackhat
-bzzzzzzzt
-cacheablemetadata
-clicksort
-closur
-configtranslation
-contrained
-crudui
-crypted
-currenttime
-currentuser
-datefield
-daycounter
-defalt
-dependening
-displayname
-distincted
-driveletter
-elit
-meφω
-oplopend
-sorteren
-toepassen
-vivamus
Remaining tasks
Scoping of issue has been agreed to by xjm. See #14.- Review
- Commit
User interface changes
API changes
Data model changes
Release notes snippet
Comment | File | Size | Author |
---|---|---|---|
#25 | 3210129-25.patch | 31.2 KB | quietone |
#25 | interdiff-23-25.txt | 637 bytes | quietone |
#23 | reroll_diff_20-23.txt | 6.38 KB | murilohp |
#23 | 3210129-23.patch | 30.7 KB | murilohp |
#20 | 3210129-20.patch | 31.91 KB | quietone |
Comments
Comment #2
quietone CreditAttribution: quietone as a volunteer commentedNot yet testing. I need to do a self review.
Comment #3
longwaveI have a feeling this will be rejected for scoping reasons, the same way we don't do things module-by-module, but I could be wrong.
We might be OK with e.g. fixing all of views.view.*.yml the same way the core.*.yml issue is quite self-contained.
Comment #4
quietone CreditAttribution: quietone as a volunteer commentedYes, finding a set of related words, self-contained, as you say is helpful and that is why I did the 'migrate' words. I was going for an easy win that would reduce the number of words in the list quickly. Sort of getting the 'noise' out of the file.
I think I'll ask the committers to comment.
Comment #5
quietone CreditAttribution: quietone as a volunteer commentedComment #7
quietone CreditAttribution: quietone as a volunteer commentedI asked xjm to about the scope on this in #contribute.
The opinion was that at 13K the current patch is too small but it partly depends on the patch author and reviewer burden. To answer those questions I will add the letter 'b' and see how long it takes and consider the reviewer perspective.
First, reroll the existing patch.
Comment #8
quietone CreditAttribution: quietone as a volunteer commentedAdding 'b'. My old list of words identified 21 that are used once in one file of which 5 have been removed in this patch. The ones not changed were those likely to be considered words, anything to do with javascript, proper name or for other reasons caused more changes. I am really trying to keep this to words the require a single code line change.
From a patch builder perspective finding these one off changes is, for me, as challenging as finding some other grouping of files or words to do.
No comment yet on reviewer perspective - I need to do something else for while.
Comment #9
quietone CreditAttribution: quietone as a volunteer commentedAdding 'c'.
Comment #10
quietone CreditAttribution: quietone as a volunteer commentedI need to review the work I have done tonight. I'll run tests after that.
Comment #11
quietone CreditAttribution: quietone as a volunteer commentedPutting my reviewer hat on I started to review the patch.
I immediately noticed that I like that the fix for each word is a one line change (perhaps more if the wrapping of a comment changed). As soon as one word is finished I know that I will not encounter it later in the patch. Sometimes that means referring back to the earlier file to check wording or something. That is avoided. Those are the only differences I see. Everything else is the same, the reviewer needs to run spellcheck on core locally and check all the added ignore lines for alphabetizing and wrapping.
The patch is currently 32KB. My feeling is that the max should be 50KB. Any other opinions on that?
Comment #12
quietone CreditAttribution: quietone as a volunteer commentedComment #13
quietone CreditAttribution: quietone as a volunteer commentedOnly 48 words, not 50
Comment #14
xjmYeah I agree that we definitely don't want patches to hit the 50K mark, so we probably shouldn't do more than three letters at a time unless one of them is "X" or something.
I'm okay with "letters starting with [_] through [_]" as a way to scope these issues, since one at a time would be a ton of overhead but the whole alphabet at a time would be unmanageably large. It is better than per-module scope because the scope is completely resolved for the words included.
Thanks @quietone!
Comment #15
quietone CreditAttribution: quietone as a volunteer commented@xjm, thank you.
I've updated the remaining task in the IS to show that this approach has committer approval.
I think this is ready for review now.
Comment #16
quietone CreditAttribution: quietone as a volunteer commentedComment #17
quietone CreditAttribution: quietone as a volunteer commentedReroll, now that #3209934: Fix spelling for 46 migrate related words was committed.
Comment #19
longwaveComment #20
quietone CreditAttribution: quietone as a volunteer commentedRerolled and then ran `yarn spellcheck:core`.
Comment #21
Spokje- Some nice catches in there ("dependening" anyone?)
- Great that it catches stuff like
cacheablemetadata
vsCacheableMetadata
- Green TestBot
RTBC for me.
Comment #22
alexpottSome patch conflicts to sort out... core/lib/Drupal/Core/Database/Driver/sqlite/Schema.php will have moved to the new sqlite module.
Comment #23
murilohp CreditAttribution: murilohp at CI&T commentedHey, I made a new reroll of patch #20 and after ran
yarn spellcheck:core
, I got the following issues:The file
drupal/core/modules/sqlite/src/Driver/Database/sqlite/Schema.php
was added recently on #3129043, so I had to rollback the following words insidecore/misc/cspell/dictionary.txt
Moving back to NR to see what do you think about it.
Thanks!
Comment #24
murilohp CreditAttribution: murilohp at CI&T commentedComment #25
quietone CreditAttribution: quietone as a volunteer commented@murilohp, thanks for the reroll and for running spellcheck on all of core. The new file core/modules/sqlite/src/Driver/Database/sqlite/Schema.php was originally /core/lib/Drupal/Core/Database/Driver/sqlite/Schema.php and was moved as part of #3129043: Move core database drivers to modules of their own. The patch in #20 had a cspell ignore line and that should be moved as well, since the goal is to remove words from dictionary.txt
This patch restores the ignore line in the sqlite Schema.php file.
Comment #26
SpokjeBack to RTBC
Comment #27
alexpottCommitted ff2d2ea and pushed to 10.0.x. Thanks!
Committed 3310654 and pushed to 9.4.x. Thanks!
I had to resolve some conflicts on 10.0.x in core/tests/Drupal/FunctionalTests/Bootstrap/UncaughtExceptionTest.php but nothing too tricky - a spelling mistake had already been removed due to not supporting PHP 7.
Comment #30
quietone CreditAttribution: quietone as a volunteer commentedFix typo in IS>