It'd be cool if core/scripts/dev/commit-code-check.sh could detect a change in the dictionary and re-run the tests. We can test if the dictionary is in $FILES - also we should only do this on DrupalCI - there's a flag for that too. Also we could extend this to:
running PHPCS on all files if phpcs.xml.dist is updated
running all JS build checks if core/yarn.lock is updated
Thus spoketh @alexpott in #3212547-7: cspell Dictionaries changed, checking all files
I think it's not only cool, but also necessary to prevent issues like #3212521: [backport] cspell dislikes identifer in core/modules/views/src/Plugin/views/filter/FilterPluginBase.php and will fail any patch touching that file
Detect a change to the dictionary and then run spellcheck:core locally and on drupalci if it is has changed.
if [[ $FILE == "core/misc/cspell/dictionary.txt" ]]; then
CSPELL_DICTIONARY_FILE_CHANGED=1;
fiReview patch #10
| Comment | File | Size | Author |
|---|---|---|---|
| #10 | 3212579-10.patch | 2.44 KB | quietone |
Show commandsStart within a Git clone of the project using the version control instructions.
Or, if you do not have SSH keys set up on git.drupalcode.org:
compare
Comments
Comment #2
wim leersIt'd also prevent un-RTBC'ing RTBC core patches like #3190815-68: Source count caching broken: impossible to enable source count caching for SqlBase-based source plugins (plus, unneeded cache I/O).
Comment #3
spokje@Wim Leers Indeed, it's always annoying to have a broken HEAD (and I use the term "broken" very loosely here, but rather as an umbrella describing "Forces Out Of Your Control Breaking Your Issue") interfering with issues.
(Not too sure
FOOYCBYIis going to be entered in the Urban Dictionary soon...)Comment #5
longwaveRan into this on #3262573: Update our yarn dev dependencies to the extent allowed by current constraints where the dictionary is now out of date but DrupalCI did not tell us.
Comment #9
quietone commentedRan into this with #2921133: Remove "Please" from the codebase. And, if this were done it would reduce the barriers for novices to contribute to spelling issues.
This patch will spell check core if dictionary.txt changes. I also modified the
spellcheck:corecommand by adding the option--no-progressso only error messages are output. That alone makes spell checking core easier. I tested locally and it seems to be working correctly.Since this is a useful addition for developers, and the lack of this has caused problems and followups I am changing this to a task.
Comment #10
quietone commentedExtra line needs to be removed
No interdiff because this is a small patch
Comment #11
_utsavsharma commentedAddressed extra line needs to be removed.
Comment #12
quietone commentedComment #13
quietone commented@_utsavsharma, The extra line was removed in #10, no work needed to be done on the patch. The comment states you were addressing the extra line but the patch removed 2 extra lines with no comment as to why. In cases, when a script is being changed we keep to the same code style so it is easy to read an maintain. Therefor, no credit for an unhelpful patch perHow is credit granted for Drupal core issues.
The original IS suggests running spellcheck:core only on drupalci. I think it should be done locally to prevent errors when working on spelling issues. It is rare and should be rare that a non-spelling issue is altering the dictionary.
Comment #14
spokjeHiding patches to make clear #10 is the current one.
Code changes make sense to me and will prevent the need for "emergency-repair-commits" when we sometimes forget to run spellcheck locally. So: RTBC.
Comment #16
catchCommitted/pushed to 11.x, thanks!
I don't think we need this in 10.1.x, but if we do, happy to cherry-pick.
Comment #18
catchWent ahead and cherry-picked now we're trying to sort out cspell linting with gitlab.