Problem/Motivation
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.
The list of words beginning with e -> i, inclusive, that are used once in one file.
Proposed resolution
Remove the following from dictionary.txt
19 words
-derp
-egroe
-fetchmode
-fffffg
-filterprovider
-firstcolumn
-formatless
-fourthcolumn
-getcode
-getfile
-getmachine
-gonner
-herpderp
-hreflangs
-htmlto
-inator
-inspectable
-secondcolumn
-thirdcolumn
Remaining tasks
Review
Commit
User interface changes
API changes
Data model changes
Release notes snippet
| Comment | File | Size | Author |
|---|---|---|---|
| #48 | 3219472-48.patch | 19.53 KB | quietone |
| #48 | interdiff-36-48.txt | 906 bytes | quietone |
| #48 | 3219472-48.patch | 19.53 KB | quietone |
| #48 | interdiff-36-48.txt | 906 bytes | quietone |
| #47 | dictionary.txt | 15.53 KB | smustgrave |
Comments
Comment #2
quietone commentedAdding e and f.
Comment #3
quietone commentedAdd 'g'.
Comment #4
quietone commentedBecause the patch is small, adding 2 more letters.
Comment #5
quietone commentedComment #6
quietone commentedComment #7
quietone commentedComment #8
quietone commentedComment #9
quietone commentedComment #11
longwaveComment #12
quietone commentedUpdated and spellchecked core.
Comment #13
quietone commentedNeeded a reroll and removed 'ftest' which was accidentally added to dictionary.txt
Comment #14
quietone commentedComment #15
karishmaamin commentedWorking on it
Comment #16
karishmaamin commentedRe-rolled patch against 9.4.x
Comment #17
longwaveThanks for rerolling. This looks close, with one question:
I am not sure that "format less normalization" makes sense, but am also not sure what a better wording would be.
Comment #18
quietone commented"for normalization without a format"?
Comment #19
longwaveWorks for me,
I re-reviewed all the other changes and they look correct so this is RTBC.
Comment #20
xjmOoh. If these were a class/trait/interface name and not a method name, it'd cause an actual bug on case-sensitive filesystems.
Likewise these:
We can also remove "thirdcolumn" from the dictionary; it also only has this one file's instance plus a fixture that is already ignored:
This is awkward. Maybe "with different hreflang attributes"?
Isn't the correct spelling "Gigabyte"?
...Ah, a comment in the test explains:
Could we move the
// cspell:ignorecomment to be above the inline comment for clarity? Or do we have a best practice of putting these declarations at the top of the file?...Does this mean that "herp" and "derp" are considered words upstream?
...No. "herp" is only in the dictonary because it is apparently short for "herptile" ("a reptile or amphibian", not what we mean), and "derp" because it's in our dictionary already. So this is essentially just moving technical debt around. Let's either ignore "herpderp" within this file, or use a different test string that meets the needs of the tests.
The word "that" is still needed here.
Hmm. I guess this is okay, but in general we shouldn't change test strings testing edgecases like this; it'd be better to add an ignore directive for them.
NW for points 2, 3, 5, 6, and maybe 4 above. Thanks!
Comment #21
longwave> Could we move the // cspell:ignore comment to be above the inline comment for clarity? Or do we have a best practice of putting these declarations at the top of the file?
I don't think we have a strict rule about this. I do wonder if we should be using
// cspell:ignore-next-linemore often, as that would reduce some technical debt if we correct or remove a misspelled word but do not remove it from the top of the file at the same time.Comment #22
andregp commented@longwave, I worked on a few related issues and I agree that defining a standard for cspell:ignore would be much better.
I personally believe that
// cspell:ignore <word>is better suitable for cases with multiple instances of the word throughout the code and// cspell:ignore-next-lineis much better when the word appear only 1-2 times.Comment #25
quietone commentedAdding tag for the cspell spelling error issues.
Comment #26
ravi.shankar commentedRerolled patched #18 on Drupal 10.1.x, keeping the status needs work for comment #20.
Comment #27
Abhishek_Singh commentedI have made two changes according to #20 point 3 & 6. Still it needs work for other points.
Comment #28
quietone commentedA new patch to address everything in #20.
1. I searched for all the changed words and found no occurrences in 10.1.x
2-4 Fixed
5. I removed 'derp' from the dictionary and made the necessary changes.
6. Also fixed. To be safe, I restored the original string and added an ignore line.
Comment #29
smustgrave commentedThis issue is being reviewed by the kind folks in Slack, #need-reveiw-queue. We are working to keep the size of Needs Review queue [2700+ issues] to around 400 (1 month or less), following Review a patch or merge require as a guide.
Reviewing the list from the IS I'm no longer finding multiple instances (most have 0) in 10.1.x
Still found strings like 'fffffg' and the column stuff "firstcolumn"
All points from #20 appear to be addressed.
Is the standard for one liners to use // cspell:disable-next-line vs cspell:ignore xyz
Comment #30
xjmCould we move this above the relevant line, as the invalid value is only declared once?
Everything else looks great to me in this patch. Nice work!
Comment #31
quietone commentedMade the change for #30.
Comment #32
smustgrave commentedRemarking
Comment #35
xjmCommitted to 10.1.x and 10.0.x, thanks!
It does not apply to 9.5.x, so we would need a different version for backport (merge conflict in the dictionary, plus there might be some instances of these words in removed code).
Comment #36
quietone commentedHere is the reroll for 9.5.x
Comment #37
quietone commentedAnd then I regenerated the dictionary.
Comment #38
xjmInteresting, how did #36 pass if there were all those words in #37 still needed?
Comment #39
xjmComment #40
xjmWhen I apply #37 and regenerate the dictionary I get a different result.
rm -rf vendor; composer installcd core; rm -rf node_modules; yarn installyarn spellcheck:make-drupal-dictI get the attached changes to the dictionary.
Comment #41
quietone commentedI followed the instructions in #40, from 9.5.x HEAD, and I do not get any changes to the dictionary.
I looked at the changes to the dictionary in #40 and find they are in yarn.lock and package.json, files that are ignored. The non English words I am not sure about.
I regenerated the dictionary on HEAD and there were changes, so it is already out of sync. Maybe that should be looked at in a separate issue.
Comment #42
quietone commentedComment #43
smustgrave commentedThis one kinda deviated from the original issue it seemed? (could be wrong) How best to review?
Comment #44
quietone commented@smustgrave, can you replicate the results that xjm got in #40.
Comment #45
smustgrave commentedApplying patch #37 on 9.5.x
Running the steps from #40
I did not get the same changes.
Comment #46
xjm@smustgrave, could you clarify which changes you do get, or upload the copy of the dictionary you got?
In particular, I'm concerned that @quietone's patch does not remove the dictionary entries for
herpderp,hreflangs, etc. which are the target of the issue.Comment #47
smustgrave commentedHere's my dictionary.txt file from #45
Comment #48
quietone commentedI went through the process of updating the dictionary for 9.5, the step in #37. This time I get different results. My only guess is that I was using a local script that didn't rm node_modules before installing yarn. However, in #40 I did not use that script and used the instructions in #40.
What I did was
Here are the results.
Comment #49
smustgrave commentedChanges look good to me.
Comment #51
longwaveI diffed #30 and #48 locally and the only changes are in dictionary.txt. I applied the patch and ran cspell locally and found no errors.
Committed and pushed to 9.5.x, thanks!