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
Becalled
Isnew
describedby this is from aria-describedby
, I don't think there's anything to fix -- this needs to stay in the word list.
Bytouched
Entitiesto
Fieldby
Groupby
Librariesby
Urlto
intefacewithconstants
invalidateby
mydistro
mydriver
myeditor
myeditoroverride
myfragment
myfrontpage
myfunctions
mymenu
mymodule - too many instances of this, this can be done separately
mynewpassword
myothermenu
mypath
myprimarykey
myprofile
myproject
myrootuser
myroute
mysetting
mysite - too many instances of this, this can be done separately
mytab
mytable
mytheme
mytube
mytype
myvalue
myvar
myverylongurl
myverylongurlexample
nosuchcolumn
nosuchindex
nosuchscheme - part of XssTest which has many spelling issues, should be done separately
nosuchtable
nosuchtag - part of XssTest which has many spelling issues, should be done separately
testid
testnoschema
wrongparam
Proposed resolution
Make sure everything isProperlyCamelCased or snake_cased as appropriate.
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 |
---|---|---|---|
#48 | 3138746-48.patch | 70.1 KB | jungle |
#48 | interdiff-42-48.txt | 1.33 KB | jungle |
Comments
Comment #2
dwwSimplifying the summary:
a) This doesn't necessarily require cspell or knowledge of how to work with it.
b) We don't need to keep being reminded that we're supposed to wrap comments to 80 chars. The pedantic dreditor review crowd will definitely make sure that happens naturally. ;)
Thanks,
-Derek
Comment #3
sja112 CreditAttribution: sja112 at Srijan | A Material+ Company for Drupal India Association commentedComment #4
sja112 CreditAttribution: sja112 at Srijan | A Material+ Company for Drupal India Association commentedAddressed
Becalled
andIsnew
,describedby
is coming in aria label.$variables['attributes']['aria-describedby'] = $description_id;
Comment #5
sja112 CreditAttribution: sja112 at Srijan | A Material+ Company for Drupal India Association commentedComment #6
longwaveSeveral more we can hopefully fix here. Some of these may need snake_case instead of camelCase.
Bytouched
Entitiesto
Groupby
Librariesby
Urlto
intefacewithconstants
invalidateby
mydistro
mydriver
myeditor
myeditoroverride
myfragment
myfrontpage
myfunctions
mymenu
mymodule
mynewpassword
myothermenu
mypath
myprimarykey
myprofile
myproject
myrootuser
myroute
mysetting
mysite
mytab
mytable
mytheme
mytube
mytype
myvalue
myvar
myverylongurl
myverylongurlexample
nosuchcolumn
nosuchindex
nosuchscheme
nosuchtable
nosuchtag
testid
testnoschema
wrongparam
Comment #7
dwwA) Moved @longwave's list from #6 into the summary. Added "or properly snake_cased" to the proposed resolution. Also removing the punctuation/formatting noise from the original list (pasted from dreditor).
B) I crossed off "describedby" since that's not camelCase nor snake_case. It's from
aria-describedby
which is the actual name of the attribute. That one should say in the word list.C) Given the changes in patch #4, I don't understand why we aren't already getting fatal errors for our tests. ;) WTF? If we misspelled the method name and are calling that in our tests, why aren't we seeing fatal errors about it? If we're not actually calling these things, why are they in the test suite at all? If we are calling them, why aren't they blowing up?
NW for the vastly expanded list, and for an answer to C. ;)
Thanks,
-Derek
Comment #8
dwwWhoops, fixing my formatting #fail.
Comment #9
longwaveOne more; Fieldby
Comment #10
sja112 CreditAttribution: sja112 at Srijan | A Material+ Company for Drupal India Association commentedComment #11
longwave> I don't understand why we aren't already getting fatal errors for our tests. ;) WTF? If we misspelled the method name and are calling that in our tests, why aren't we seeing fatal errors about it?
PHP is not case sensitive for function, method or variable names, so
$this->shouldbecamelcased()
is identical to$this->shouldBeCamelCased()
. So there is no technical issue here, but fixing this improves readability.Comment #12
sja112 CreditAttribution: sja112 at Srijan | A Material+ Company for Drupal India Association commentedOn this.
Comment #13
sja112 CreditAttribution: sja112 at Srijan | A Material+ Company for Drupal India Association commentedI have included the changes for the list of words shared in #6.
I have not changed many words where in my view change was not required. Below is the list,
intefacewithconstants
mydistro
mydriver
myeditor
myeditoroverride
myfragment
myfrontpage
myfunctions
mymenu
mymodule
mynewpassword
myothermenu
mypath
myprimarykey
myprofile
myproject
myrootuser
myroute
mysite
mytab
mytable
mytube
myvalue
myverylongurl
myverylongurlexample
nosuchcolumn
nosuchindex
nosuchscheme
nosuchtable
nosuchtag
testid
testnoschema
wrongparam
Comment #14
jungle#2972224: Add .cspell.json to automate spellchecking in Drupal core landed, let's remove typos from core/misc/cspell/dictionary.txt to avoid making the same mistakes again.
Comment #15
rajandro CreditAttribution: rajandro as a volunteer and at Srijan | A Material+ Company for Drupal India Association commentedI am working on this.
Comment #16
rajandro CreditAttribution: rajandro as a volunteer and at Srijan | A Material+ Company for Drupal India Association commentedI have update core/misc/cspell/dictionary.txt by removing the above-listed typos from the list. Please review the changes.
Comment #17
longwaveI am not sure about this one. JavaScript is case sensitive and so technically changing the T here might cause a bug in the code. But the property doesn't seem used anywhere else, so why does it exist in the first place? Even a case insensitive search for "forbiddenpropertyrule" only finds these two lines.
Probably out of scope here but I wonder if we can find a way of enforcing initialisms and acronyms such as "CSS" being correctly capitalised.
"fieldapi" should be really be "Field API" which is technically in scope of this issue.
Dataprovider -> Data provider
Comment #18
rajandro CreditAttribution: rajandro as a volunteer and at Srijan | A Material+ Company for Drupal India Association commentedOk, Let me work on #17
Comment #19
rajandro CreditAttribution: rajandro as a volunteer and at Srijan | A Material+ Company for Drupal India Association commentedI have addressed point 1, 3, 4 of comment #17 where point 2 kept as it is as I think we can create a separate ticket for this.
Also in addition to the above update, I have noticed
* Dataprovider
in near about 30 places I have changed it to* Data provider
Thanks
Rajandro
Comment #20
jungle.js file(s) should be generated by
cd core && yarn && yarn build:js
Comment #21
jungleAlso tagging "Global2020"
Comment #22
jameszhang023 CreditAttribution: jameszhang023 commentedworking on this
Comment #23
jameszhang023 CreditAttribution: jameszhang023 commentedRerollling done, please review, thanks.
Comment #24
jungle@jameszhang023, thanks for rerolling!
Spelling check did not pass, and did not see removals of my* words from the directory in #23
Comment #25
longwaveRegenerated the dictionary, as the Annotation component that copies Doctrine was ignored in #3138766: Fix "Don't" relevant typos in core and this removes several words including some of the ones identified here.
I have left mymodule and mysite alone for now (except on lines touched by other changes in this patch) as they are used in a large number of places. I also left nosuchscheme and nosuchtag as I think we should deal with XssTest in a separate issue, it has many words we could ignore.
Comment #26
jungleThanks @longwave!
Addressing:
.js file should be generated by running
yarn build:js
Comment #27
jungleChange the title/scope slightly to inline with the changes here/patch.
Comment #28
jungleAdding the total number
Comment #29
dwwMostly looks great.
I had a brief scare about changing function names, but then I remembered that PHP is case insensitive. ;)
A few concerns:
Are these getting fixed here? I don't see them. Seems out of scope.
If we're touching these anyway, why not entirely remove the redundant messages in the final arg?
Comment #30
longwaveupserting was removed in #3129560: Remove the Upsert implementation for PostgreSQL < 9.5
Neither of these issues regenerated the dictionary. Perhaps this task should be spun out to a separate issue?
Also updated IS to remove words that I think should be handled elsewhere.
Comment #31
jungleRe #29.1/#30.1 filed #3164211: Remove 8 fixed typos from misc/cspell/dictionary.txt
Comment #32
jungledistro
is a new unknown word from fixingmydistro
assertEqual()
withassertEquals()
at the same time.Comment #33
junglePostpone on #3164211
Comment #34
jungle#32 was built upon #3164211, this patch includes #3164211.
Comment #36
jungleFixing test failed.
Comment #37
quietone CreditAttribution: quietone as a volunteer commentedDo not change anything in the Drupal 7 test fixture.
The migrate fixtures, drupal6.php and drupal7.php, are in the ignoredPaths in .cspell.json.
Comment #38
jungleGood point, @quietone, thanks! Reverted.
Comment #39
jungle#3164211: Remove 8 fixed typos from misc/cspell/dictionary.txt was in. Rerolled.
Comment #40
junglePlease ignore #39. Rerolled from #38 again.
Comment #42
quietone CreditAttribution: quietone as a volunteer commentedNeeded a reroll.
Comment #43
Abhijith S CreditAttribution: Abhijith S as a volunteer and at Zyxware Technologies commentedApplied patch #42 . Works fine on 9.2.x
Comment #44
jungleLooks great. thanks!
Comment #45
jungleNeeds followup to remove leftovers in
misc/cspell/dictionary.txt
Comment #46
jungleRemoving the Needs followup tag
Comment #47
alexpottThis was asked in #17.1 but there was no reply - JS is case sensitive so some sleuthing needs to take place to explain what it go on here. It was added in #1894644: Unidirectional editor configuration -> filter settings syncing. I think this is dead code that was in previous iterations of the patch a so should be removed. Imo should file an issue to do that rather than change it here. It could block this one or we could remove the change and revert the
bytouched
fix in the dictionary.Comment #48
jungleSorry, missed #17.1, reverting. Tagging needs followup for #17.1
Comment #49
jungleFiled #3181290: Remove dead code in editor.admin.es6.js involving touchedBytouchedByForbiddenPropertyRule and setting back to RTBC
Comment #50
longwaveComment #51
alexpottCommitted and pushed 0eec7fe50d to 9.2.x and a5dac86510 to 9.1.x. Thanks!
Backported to 9.1.x since this only changes the case of things in runtime code and documentation. There are test changes too but not things that are part of the API.