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

Comments

quietone created an issue. See original summary.

quietone’s picture

Issue summary: View changes
Status: Needs review » Active
StatusFileSize
new12.59 KB

Adding e and f.

quietone’s picture

Issue summary: View changes
StatusFileSize
new3.02 KB
new16.19 KB

Add 'g'.

quietone’s picture

Issue summary: View changes
StatusFileSize
new5.55 KB
new22.97 KB

Because the patch is small, adding 2 more letters.

quietone’s picture

Title: Fix spelling for words used once, beginning with 'e' -> 'g', inclusive » Fix spelling for words used once, beginning with 'e' -> 'i', inclusive
quietone’s picture

Issue summary: View changes
Status: Active » Needs review
StatusFileSize
new911 bytes
quietone’s picture

Issue summary: View changes
quietone’s picture

Assigned: quietone » Unassigned

Version: 9.3.x-dev » 9.4.x-dev

Drupal 9.3.0-rc1 was released on November 26, 2021, which means new developments and disruptive changes should now be targeted for the 9.4.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

longwave’s picture

Status: Needs review » Needs work
Issue tags: +Needs reroll
quietone’s picture

Issue summary: View changes
Status: Needs work » Needs review
Issue tags: -Needs reroll
StatusFileSize
new6.88 KB
new21.14 KB

Updated and spellchecked core.

quietone’s picture

StatusFileSize
new2.46 KB
new20.2 KB

Needed a reroll and removed 'ftest' which was accidentally added to dictionary.txt

quietone’s picture

Status: Needs review » Needs work
Issue tags: +Needs reroll
karishmaamin’s picture

Assigned: Unassigned » karishmaamin

Working on it

karishmaamin’s picture

Assigned: karishmaamin » Unassigned
Status: Needs work » Needs review
StatusFileSize
new19.15 KB

Re-rolled patch against 9.4.x

longwave’s picture

Issue tags: -Needs reroll

Thanks for rerolling. This looks close, with one question:

+++ b/core/modules/jsonapi/src/Normalizer/NormalizerBase.php
@@ -62,7 +62,7 @@ protected static function rasterizeValueRecursive($value) {
-    // for formatless normalization. The JSON:API module wants to be cautious.
+    // for format less normalization. The JSON:API module wants to be cautious.

I am not sure that "format less normalization" makes sense, but am also not sure what a better wording would be.

quietone’s picture

StatusFileSize
new1.02 KB
new19.54 KB

"for normalization without a format"?

longwave’s picture

Status: Needs review » Reviewed & tested by the community

Works for me,

I re-reviewed all the other changes and they look correct so this is RTBC.

xjm’s picture

Status: Reviewed & tested by the community » Needs work
  1. +++ b/core/modules/editor/src/Plugin/InPlaceEditor/Editor.php
    @@ -60,7 +60,7 @@ public function getMetadata(FieldItemListInterface $items) {
    -    return (bool) count(array_intersect([FilterInterface::TYPE_TRANSFORM_REVERSIBLE, FilterInterface::TYPE_TRANSFORM_IRREVERSIBLE], $format->getFiltertypes()));
    +    return (bool) count(array_intersect([FilterInterface::TYPE_TRANSFORM_REVERSIBLE, FilterInterface::TYPE_TRANSFORM_IRREVERSIBLE], $format->getFilterTypes()));
    
    +++ b/core/modules/user/tests/src/Functional/UserPictureTest.php
    @@ -116,7 +116,7 @@ public function testPictureOnNodeComment() {
    -    $image_url = \Drupal::service('file_url_generator')->transformRelative($style->buildUrl($file->getfileUri()));
    +    $image_url = \Drupal::service('file_url_generator')->transformRelative($style->buildUrl($file->getFileUri()));
    

    Ooh. 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:

    +++ b/core/modules/system/tests/src/Functional/Mail/HtmlToTextTest.php
    @@ -194,7 +194,7 @@ public function testDrupalHtmlToTextArgs() {
    -  public function testDrupalHtmltoTextCollapsesWhitespace() {
    +  public function testDrupalHtmlToTextCollapsesWhitespace() {
    
    +++ b/core/tests/Drupal/KernelTests/Core/Entity/EntityApiTest.php
    @@ -211,7 +211,7 @@ public function testEntityStorageExceptionHandling() {
    -      $this->assertEquals(1, $e->getcode(), 'Entity presave EntityStorageException caught.');
    +      $this->assertEquals(1, $e->getCode(), 'Entity presave EntityStorageException caught.');
    
    +++ b/core/tests/Drupal/Tests/Core/Authentication/AuthenticationManagerTest.php
    @@ -49,7 +49,7 @@ public function testDefaultFilter($applies, $has_route, $auth_option, $provider_
    -  public function testApplyFilterWithFilterprovider() {
    +  public function testApplyFilterWithFilterProvider() {
    
  2. +++ b/core/modules/block/migrations/d7_block.yml
    @@ -1,3 +1,4 @@
    +# cspell: ignore firstcolumn secondcolumn thirdcolumn fourthcolumn
    

    We can also remove "thirdcolumn" from the dictionary; it also only has this one file's instance plus a fixture that is already ignored:

    [ayrton:maintainer | Mon 11:58:32] $ grep -rl "thirdcolumn" *
    core/modules/migrate_drupal/tests/fixtures/drupal7.php
    core/modules/block/migrations/d7_block.yml
  3. +++ b/core/modules/system/tests/src/Functional/Render/HtmlResponseAttachmentsTest.php
    @@ -69,7 +69,7 @@ public function testAttachments() {
    -    // Check that duplicate alternate URLs with different hreflangs are allowed.
    +    // Check that duplicate alternate URLs with different hreflang are allowed.
    

    This is awkward. Maybe "with different hreflang attributes"?

  4. +++ b/core/tests/Drupal/Tests/Component/Utility/BytesTest.php
    @@ -6,6 +6,8 @@
    +// cspell:ignore giggabyte
    

    Isn't the correct spelling "Gigabyte"?

    ...Ah, a comment in the test explains:

          // 76.24 GB (with typo).                                                  
    

    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?

  5. +++ b/core/tests/Drupal/KernelTests/Core/Config/ConfigOverrideTest.php
    @@ -93,7 +93,7 @@ public function testConfOverride() {
    -      '404' => 'herpderp',
    +      '404' => 'herp derp',
    

    ...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.

  6. +++ b/core/tests/Drupal/KernelTests/Core/Plugin/InspectionTest.php
    @@ -3,7 +3,7 @@
    - * Tests that plugins implementing PluginInspectionInterface are inspectable.
    + * Tests plugins implementing PluginInspectionInterface can be inspected.
    

    The word "that" is still needed here.

  7. +++ b/core/tests/Drupal/Tests/Core/File/FileSystemTest.php
    @@ -166,7 +166,7 @@ protected function assertFilePermissions(int $expected_mode, string $uri, string
    -    $filename = "a\xFFsdf\x80€" . '.txt';
    +    $filename = "a\xFFFoo\x80€" . '.txt';
    

    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!

longwave’s picture

> 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-line more 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.

andregp’s picture

@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-line is much better when the word appear only 1-2 times.

Version: 9.4.x-dev » 9.5.x-dev

Drupal 9.4.0-alpha1 was released on May 6, 2022, which means new developments and disruptive changes should now be targeted for the 9.5.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

Version: 9.5.x-dev » 10.1.x-dev

Drupal 9.5.0-beta2 and Drupal 10.0.0-beta2 were released on September 29, 2022, which means new developments and disruptive changes should now be targeted for the 10.1.x-dev branch. For more information see the Drupal core minor version schedule and the Allowed changes during the Drupal core release cycle.

quietone’s picture

Issue tags: +cspell error

Adding tag for the cspell spelling error issues.

ravi.shankar’s picture

StatusFileSize
new17.21 KB
new13.36 KB

Rerolled patched #18 on Drupal 10.1.x, keeping the status needs work for comment #20.

Abhishek_Singh’s picture

StatusFileSize
new1.35 KB
new17.21 KB

I have made two changes according to #20 point 3 & 6. Still it needs work for other points.

quietone’s picture

Issue summary: View changes
Status: Needs work » Needs review
StatusFileSize
new4.79 KB
new19.01 KB

A 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.

smustgrave’s picture

Status: Needs review » Reviewed & tested by the community

This 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

xjm’s picture

Status: Reviewed & tested by the community » Needs review
+++ b/core/modules/system/tests/src/Functional/Form/FormTest.php
@@ -14,6 +14,8 @@
+// cspell:ignore fffffg
+

Could 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!

quietone’s picture

StatusFileSize
new752 bytes
new19.08 KB

Made the change for #30.

smustgrave’s picture

Status: Needs review » Reviewed & tested by the community

Remarking

  • xjm committed 1d6b90e8 on 10.1.x
    Issue #3219472 by quietone, Abhishek_Singh, karishmaamin, longwave,...

  • xjm committed 26676758 on 10.0.x
    Issue #3219472 by quietone, Abhishek_Singh, karishmaamin, longwave,...
xjm’s picture

Version: 10.1.x-dev » 9.5.x-dev
Status: Reviewed & tested by the community » Needs work

Committed 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).

quietone’s picture

Status: Needs work » Needs review
StatusFileSize
new2.16 KB
new19.14 KB

Here is the reroll for 9.5.x

quietone’s picture

StatusFileSize
new1.74 KB
new18.72 KB

And then I regenerated the dictionary.

xjm’s picture

Interesting, how did #36 pass if there were all those words in #37 still needed?

xjm’s picture

StatusFileSize
new486 bytes
xjm’s picture

Status: Needs review » Needs work
StatusFileSize
new1.97 KB

When I apply #37 and regenerate the dictionary I get a different result.

  1. Apply #37 to 9.5.x HEAD.
  2. rm -rf vendor; composer install
  3. cd core; rm -rf node_modules; yarn install
  4. yarn spellcheck:make-drupal-dict

I get the attached changes to the dictionary.

quietone’s picture

I 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.

quietone’s picture

Status: Needs work » Needs review
smustgrave’s picture

This one kinda deviated from the original issue it seemed? (could be wrong) How best to review?

quietone’s picture

@smustgrave, can you replicate the results that xjm got in #40.

smustgrave’s picture

Applying patch #37 on 9.5.x
Running the steps from #40

I did not get the same changes.

xjm’s picture

@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.

smustgrave’s picture

StatusFileSize
new15.53 KB

Here's my dictionary.txt file from #45

quietone’s picture

StatusFileSize
new906 bytes
new19.53 KB
new906 bytes
new19.53 KB

I 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

  1. install the re-roll, #36.
  2. use script to install (which rm -rf vendor and node_modules)
  3. generate the dictionary.

Here are the results.

smustgrave’s picture

Status: Needs review » Reviewed & tested by the community

Changes look good to me.

  • longwave committed aac629ad on 9.5.x
    Issue #3219472 by quietone, xjm, Abhishek_Singh, karishmaamin,...
longwave’s picture

Status: Reviewed & tested by the community » Fixed

I 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!

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.