Fix cspell use: specify globRoot and always pass --root to cspell

Matching ✨ Make it possible for contrib modules to reuse core's commit-code-check.sh Needs work .

But clearly the #5 output fails like @bnjmnm says…

Managed to reproduce this after >30 minutes of trying! I'm 51% confident it's because of $TOP_LEVEL in

+++ b/core/scripts/dev/commit-code-check.sh
@@ -200,7 +200,7 @@
-yarn run -s spellcheck --no-must-find-files -c $TOP_LEVEL/core/.cspell.json $ABS_FILES
+yarn run -s spellcheck --no-must-find-files -c $TOP_LEVEL/core/.cspell.json --root $TOP_LEVEL $ABS_FILES

… which is /path/to/drupal/core, but .cspell.json lives in /path/to/drupal/core/core, and yarn run -s spellcheck is executed from /path/to/drupal/core/core, which is why in HEAD it works fine: because all ignorePaths in .cspell.json are also relative to /path/to/drupal/core/core!

If we really want to change nothing, then we have to use --root $TOP_LEVEL/core AFAICT. Let's find out 🤓

it will only detect them in the first place if the current working directory is above all of those absolute paths.

Uh oh, apparently DrupalCI is currently broken, so #8 will pass, but wrongly so … stay tuned at https://drupal.slack.com/archives/C51GNJG91/p1677000744230719.

I have a full solution for this, after experimenting with this locally for hours, because I really want to see ✨ Make it possible for contrib modules to reuse core's commit-code-check.sh Needs work land. Can't finish the write-up today though.

🐛 Fix commit-code-check.sh on DrupalCI Fixed landed overnight to address #9 🥳 Re-testing #8.

Yay, #8 now fails as expected, meaning that what I wrote in #8 is correct. Now let's get the proper fix in place…

While #8 works, now this patch is completely missing the point that @phenaproxima made at the very start:

it will only detect them in the first place if the current working directory is above all of those absolute paths.

😬

Discrepancies between commit-code-check.sh (used by DrupalCI and git pre-commit hooks) and yarn run spellcheck:core

I've discovered that core/scripts/dev/commit-code-check.sh has been the driving force to update core/.cspell.json, not yarn run spellcheck:core. Even though that is the officially recommended way to run spellchecks on core during local development — see https://www.drupal.org/docs/develop/automated-testing/run-core-developme... → !

And there are significant differences between the two:

Drupal's use of cspell is pretty confusing.

How did this happen?

Unclear. But the fact is that we updated to cspell 5 ( #3226052: Update to cspell 5 → ) and then 6 ( #3294850: Update to cspell 6 → ) without significant changes in our core/.cspell.json, while the changelog for those 2 versions has countless changes/bugfixes for handling: A) root, B) globbing, C) config. Just grep it for any of those keywords.

In fact, #3226052 brought in cspell 5.12.3 which is the first version in which a huge fix for root handling shipped Quite possibly that is the first version in which the problem that @phenaproxima is reporting here started happening. But that fix seems to have been specifically because they first made 5.3.0-alpha.0 interpret the ignorePaths configuration in a sane way, with a newly supported optional globRoot configuration entry, but got it wrong for some cases.

Long story short: it's broken, we gotta fix it.

Possible solutions

We either:

1. Always assume --root gets passed in, then we could just update all ignorePaths … but that means they won't work for other roots! In other words: it's brittle. (And it'll hence also mean ✨ Make it possible for contrib modules to reuse core's commit-code-check.sh Needs work will have to change this again.)
2. Alternatively: keep the same exact ignorePaths, but just specify the new globRoot.

⇒ I'm going with solution 2

Step 1: specifying globRoot, and reverting #8's changes.

If my patch is correct, it should fail just like #8 and not like #5.

That worked 👍

Step 2: make yarn spellcheck:core also specify --root, so that it's consistent with how DrupalCI runs it, and to ensure we hopefully never regress again.

That worked 👍

Step 3: improve composability. Move the specifying of the configuration out of spellcheck:core and into spellcheck — because the whole point is of course to use core's configuration whenever you use yarn run spellcheck from within the core directory.

⚠️ If you're wondering "well what about contrib modules then?!", well, good news! Thanks to PR 966 shipped in 5.3.0-alpha0, local configuration files are always loaded!

So if I want to validate only the src directory and composer.json in my contrib module using core's command, I get:

$ yarn run spellcheck --no-progress --root ../modules/contrib/automatic_updates composer.json src/**
yarn run v1.22.19
$ cspell -c .cspell.json --no-progress --root ../modules/contrib/automatic_updates composer.json 'src/**'
/Users/wim.leers/core/modules/contrib/automatic_updates/composer.json:40:6 - Unknown word (colinodell)
/Users/wim.leers/core/modules/contrib/automatic_updates/composer.json:40:21 - Unknown word (testlogger)
/Users/wim.leers/core/modules/contrib/automatic_updates/src/Updater.php:11:42 - Unknown word (zpelling)
CSpell: Files checked: 37, Issues found: 3 in 2 files
error Command failed with exit code 1.
info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.

and if I then do

echo '{ "ignoreWords": ["testlogger"] }' > ../modules/contrib/automatic_updates/.cspell.json

to create a "local" configuration file, then that is respected:

$ yarn run spellcheck --no-progress --root ../modules/contrib/automatic_updates composer.json src/**
yarn run v1.22.19
$ cspell -c .cspell.json --no-progress --root ../modules/contrib/automatic_updates composer.json 'src/**'
/Users/wim.leers/core/modules/contrib/automatic_updates/composer.json:40:6 - Unknown word (colinodell)
/Users/wim.leers/core/modules/contrib/automatic_updates/src/Updater.php:11:42 - Unknown word (zpelling)
CSpell: Files checked: 37, Issues found: 2 in 2 files
error Command failed with exit code 1.
info Visit https://yarnpkg.com/en/docs/cli/run for documentation about this command.

🤩

That worked 👍

Step 4: simplify commit-code-check.sh thanks to step 3.

End result:

• local development: yarn run spellcheck:core, which contains yarn spellcheck --root .. "core/**/*" "composer/**/*" "composer.json" → checks all of Drupal core
• DrupalCI: commit-code-check.sh: yarn run -s spellcheck --no-must-find-files --root $TOP_LEVEL $ABS_FILES → checks only changed files in Drupal core, and does not fail if only ignored files have changed

… and as shown in #16's output, this simultaneously unblocks ✨ Make it possible for contrib modules to reuse core's commit-code-check.sh Needs work because it is able to apply core's spellcheck rules to contrib modules that choose to use this script, because --root $TOP_LEVEL would stop pointing to the top level of the Drupal core git repo, but to the top level of the contrib module's git repo.

(And clearly, this means that Drupal core's yarn run spellcheck also becomes usable for contrib modules!)

Patch for #17.

That worked 👍

Reverting the intentional zpelling mistakes @bnjmnm introduced for testing purposes … 🤓

Tested by adding some junk into action.module. "ewfdsafdsa"
Running npm run spellcheck:core flags it correctly
Running ./core/scripts/dev/commit-code-check.sh also flagged it.

The last submitted patch, 19: 3314151-19.patch, failed testing. View results →

That smells like a random failure. Restoring RTBC.

Committed to 10.1.x. I'm thinking no backport as contrib modules that use cspell may suddenly get several new cspell fails, I'm thinking it's best to keep those new fails happening in the upcoming minor so it doesn't disrupt tests being run against the current 10.0 release. Still open to reasons why moving this to 10.0.x might be good, though.

I actually don't see how this could cause contrib failures, since this script is literally only used by Drupal core today? 😅

Per https://www.drupal.org/drupalorg/docs/drupal-ci/customizing-drupalci-tes... → , DrupalCI only uses core's run-tests.sh, not core's commit-code-check.sh, which is what this issue/patch modified!

This just opens the door to making it easy for contrib to start adopting core's commit-code-check.sh optionally! Which is what ✨ Make it possible for contrib modules to reuse core's commit-code-check.sh Needs work is about, and this now unblocked! 🥳

So IMHO this is totally fine to backport to 10.0.x :)

bnjmnm → closed merge request !2840

Confirmed with @Wim Leers that the projects manipulating commit-code-check.sh in a manner that this change could disrupt are limited, and ones that we're directly involved with. This change unblocks a much better approach than what either of these project are doing, so this can be backported to 10.0.x.

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