Early Bird Registration for DrupalCon Portland 2024 is open! Register by 23:59 PST on 31 March 2024, to get $100 off your ticket.
Problem/Motivation
Discovered at #2972224: Add .cspell.json to automate spellchecking in Drupal core, and pointed by @xjm in https://www.drupal.org/project/drupal/issues/3122088#comment-13628724
+++ b/core/.cspell.json
@@ -0,0 +1,1288 @@
+ "DONT",
...
+ "Dont",
...
+ "don't",
@xjm:Unless this is French, it should probably be DoNot/DO_NOT in code and Don't or Do not in text. There may be a whole category in here of contractions without their apostrophes.
Proposed resolution
As title and see the change record https://www.drupal.org/node/3122084 for how to work with cspell.
Remaining tasks
Pick out all applicable words from #2972224: Add .cspell.json to automate spellchecking in Drupal core and fix them.
User interface changes
API changes
Data model changes
Release notes snippet
Comment | File | Size | Author |
---|---|---|---|
#36 | 3138766-36.patch | 10.93 KB | Lal_ |
#35 | interdiff.txt | 2.83 KB | Lal_ |
#30 | raw-interdiff-27-30.txt | 1.18 KB | jameszhang023 |
#30 | 3138766-30.patch | 10.93 KB | jameszhang023 |
Comments
Comment #2
jungleComment #3
mohrerao CreditAttribution: mohrerao as a volunteer and at Srijan | A Material+ Company for Drupal India Association commentedComment #4
mohrerao CreditAttribution: mohrerao as a volunteer and at Srijan | A Material+ Company for Drupal India Association commentedComment #5
mohrerao CreditAttribution: mohrerao as a volunteer and at Srijan | A Material+ Company for Drupal India Association commentedComment #6
mohrerao CreditAttribution: mohrerao as a volunteer and at Srijan | A Material+ Company for Drupal India Association commentedComment #7
longwaveThere are more instances of "dont", they appear in code and we need to change these to "do_not" etc
Comment #8
mohrerao CreditAttribution: mohrerao as a volunteer and at Srijan | A Material+ Company for Drupal India Association commented@longwave
This may impact some of the test methods which may have been used in other modules/projects/tests.
Also found one instance of url: https://www.drupal.org/docs/8/upgrade/preparing-an-upgrade#dont_create_c....
Shall i go ahead and update?
Comment #9
longwaveSee https://www.drupal.org/core/d8-bc-policy#tests for the policy regarding tests. Basically we can change individual tests as necessary except for base classes and helpers.
The URL will probably have to be left alone for now.
Comment #10
mohrerao CreditAttribution: mohrerao as a volunteer and at Srijan | A Material+ Company for Drupal India Association commented@longwave. Thanks for the info. Attaching updated patch.
Comment #11
mohrerao CreditAttribution: mohrerao as a volunteer and at Srijan | A Material+ Company for Drupal India Association commentedComment #12
sja112 CreditAttribution: sja112 at Srijan | A Material+ Company for Drupal India Association commentedComment #13
longwaveThis covers all cases of "dont" or "DONT" in core except one in a d.o URL anchor that we can't change. There are no BC issues because everything is either in test code or CSS comments. We are changing core/tests/Drupal/Tests/Component/Annotation/Doctrine/DocParserTest.php which we inherited from Doctrine but I think a minor spelling fix is acceptable here. Therefore, this is RTBC.
Comment #14
xjmFor these, I verified that the title data is not ever actually used in the test; it's just meant to distinguish which scenarios meet the logic requirements vs. not. So that's all good.
I also read over
MigrateSkipRowTest
andSessionTest
and their fixtures rather carefully to make sure that the changes to the fixtures didn't alter the test coverage in any way.Meanwhile, I updated this URL fragment in the handbook:
https://www.drupal.org/docs/upgrading-drupal/preparing-a-site-for-upgrad...
I left an
<a>
tag with the old name/ID in place also, so that anything else that had the old link would not be broken. Confirmed the link works as expected.Updating that URL is also technically backportable since it's one of the placeholder values of the string, rather than part of the first argument of
t()
, so it's not a string break. That said, for good issue scoping, we should still probably separate the test-only changes here from that change.While @longwave is correct that this would be harmless to change, this file is actually a coding-standards-ignored file, so it should also be a cspell-ignored file IMO. In fact, the entire component should be as well as its test namespace. Can we remove this change from the patch, and incorporate that back into the parent issue?
I grepped the rest of our codebase and... the remaining instances are in fact the French word "dont". La chose exacte dont je parlais! 🇫🇷
NW for that last bullet and also to file the followup for the string URL correction. Thanks!
Comment #15
sja112 CreditAttribution: sja112 at Srijan | A Material+ Company for Drupal India Association commentedOn this.
Comment #16
sja112 CreditAttribution: sja112 at Srijan | A Material+ Company for Drupal India Association commentedAddressing #14.4
Comment #17
sja112 CreditAttribution: sja112 at Srijan | A Material+ Company for Drupal India Association commentedFiled: #3143724: Fix "dont" relevant typos in URL string
Comment #18
mohrerao CreditAttribution: mohrerao as a volunteer and at Srijan | A Material+ Company for Drupal India Association commentedRan
grep -ri "Dont" *
and could find occurance of dont in translations and in /vendor/ckeditor/ckeditor.js which should be OKRTBC for me
Comment #19
xjmCSpell landed today, so we can make sure this spelling error never happens again by removing the entry from Drupal's dictionrary, so let's update the patch to include that. I think the French content should already be ignored, but if not, we should ignore the translations where it appears.
We should also add all of
core/tests/Drupal/Tests/Component/Annotation/Doctrine
to the ignored files. Thanks!Comment #20
nikitagupta CreditAttribution: nikitagupta at Srijan | A Material+ Company for Drupal India Association commentedComment #21
mohrerao CreditAttribution: mohrerao as a volunteer and at Srijan | A Material+ Company for Drupal India Association commentedRemoved 'dont' from core/misc/cspell/dictionary.txt and added core/tests/Drupal/Tests/Component/Annotation/Doctrine to the ignored files.
Comment #22
mohrerao CreditAttribution: mohrerao as a volunteer and at Srijan | A Material+ Company for Drupal India Association commentedComment #23
nikitagupta CreditAttribution: nikitagupta at Srijan | A Material+ Company for Drupal India Association commentedComment #24
longwaveThese paths are relative to the "core" directory so we don't need "core/" at the start of the path.
Comment #25
ravi.shankar CreditAttribution: ravi.shankar at OpenSense Labs commentedHere I have addressed comment #24.
Comment #26
longwaveWe also need to add
/**
to ignore all the files below that directory, and I took the liberty of keeping the (mostly) alphabetical order of the exclusions as well.Comment #27
longwaveSo we should also regenerate the dictionary, as there are a number of misspelled words that only appear in the Doctrine component. The attached patch removes those words from the dictionary as well as "dont".
Comment #28
junglecd core && yarn && yarn build:css
and confirmed the css file touched is generated.Comment #29
jameszhang023 CreditAttribution: jameszhang023 commentedWorking on this.
Comment #30
jameszhang023 CreditAttribution: jameszhang023 commentedRolling done, please review, thanks.
Comment #31
jungleSpelling check passed.
There are CSS-related changes, checked with
cd core && yarn build:css
, confirmed that changes are all good.5 words removed in total.
Thanks!
Comment #33
jungleRandom failure, re-queued.
Comment #34
catchNeeds a reroll.
Comment #35
Lal_rerolled
Comment #36
Lal_oops the patch
Comment #37
longwaveThanks for rerolling, back to RTBC.
Comment #38
alexpottCommitted 5412462 and pushed to 9.1.x. Thanks!
Committed and pushed 3b668ff177 to 9.0.x and 3a730ce872 to 8.9.x. Thanks!
Backported to 8.9.x without the cspell changes as this is test only.