Problem/Motivation

Why is cspell running on composer/Metapackage/CoreRecommended/composer.json

Steps to reproduce

https://git.drupalcode.org/project/drupal/-/jobs/10633584#L65

Proposed resolution

Ignore composer.json and composer.lock files.

Remaining tasks

User interface changes

Introduced terminology

API changes

Data model changes

Release notes snippet

Issue fork drupal-3607061

Command icon Show commands

Start 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:

About issue forks

Comments

mstrelan created an issue. See original summary.

quietone’s picture

quietone commented

Because it has a description field that is English text.

quietone’s picture

quietone commented

But I take your point. I would think we can get a one line description correct.

mstrelan’s picture

mstrelan commented

The problem here is that bumping one of core's dependencies introduced a new transient dependency on symfony/polyfill-deepclone, and cspell doesn't like the word "deepclone". You could argue that cspell helped us to notice a new dependency was added, but of course that wouldn't happen if the new dependency was a dictionary word. You could also argue that since we're adding deepclone as a dependency, this is a good prompt to tell us we might want to put it in the dictionary, as we might refer to it in other places.

On the other hand, it felt frustrating that this interrupted my workflow, and I'm not sure it was worthwhile.

TL;DR this should be up for discussion rather than just a matter of just excluding this file from cspell to make it easier.

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented

+1 - looks like we could remove various other non-words from dictionary.txt if we do this such as "colinodell", "spaze", "lullabot" etc.

Given that composer.json is mostly machine readable except for "description" and maybe one or two other keys, to me means any disruption caused outweighs any benefits here.

mstrelan opened merge request !16181

mstrelan’s picture

mstrelan commented
Status: Active » Needs review

Updated config and dictionaries. LLM assisted here to identify what we could remove. Only two words were removable due to excluding composer.json, the others were already removable. Have used separate commits to highlight which ones are due to this config change.

longwave’s picture

longwave
he/him
Primary language English
Location UK
commented
Status: Needs review » Needs work

Added a question.

mstrelan’s picture

mstrelan commented
Status: Needs work » Needs review

Added glob pattern

smustgrave’s picture

smustgrave commented
Status: Needs review » Reviewed & tested by the community

Seems feedback from @longwave has been applied.

Random looking at the list seems maybe the dictionary.txt has gotten behind, example lzma was removed but wasn't in composer.json. Almost wonder if there's a weekly job that could be ran to check that? Not in scope here obviously just thinking out loud.

  • catch committed 7150d7e9 on main 
    fix: #3607061 Don't spellcheck composer.json

By: mstrelan
By: quietone...
catch’s picture

catch
he/him
Primary language English
commented
Version: main » 11.4.x-dev
Status: Reviewed & tested by the community » Fixed

Committed/pushed to main, thanks! Also cherry-picked to 11.x and 11.4.x

Now that this issue is closed, review the contribution record.

As a contributor, attribute any organization that helped you, or if you volunteered your own time.

Maintainers, credit people who helped resolve this issue.

catch closed merge request !16181

  • catch committed 13afc2ed on 11.4.x 
    fix: #3607061 Don't spellcheck composer.json

By: mstrelan
By: quietone...

  • catch committed 9e46fcbf on 11.x 
    fix: #3607061 Don't spellcheck composer.json

By: mstrelan
By: quietone...
godotislate’s picture

godotislate
he/him
commented
Status: Fixed » Needs work

This broke 11.x and 11.4.x because Drupal\Core\Archiver\Tar, which was removed in main, has usages of lzma.

See
https://git.drupalcode.org/project/drupal/-/jobs/11120775
https://git.drupalcode.org/project/drupal/-/jobs/11120802

godotislate opened merge request !16415

godotislate’s picture

godotislate
he/him
commented
Status: Needs work » Needs review

https://git.drupalcode.org/project/drupal/-/merge_requests/16415

xjm’s picture

xjm
she/her
Primary language English
commented
Status: Needs review » Reviewed & tested by the community

Came here to say that instead of not-spellchecking composer.json, I thought the correct approach was adding project names to the relevant dictionary. But it looks like in this case the backport did not match what was needed for those branches, correct?

The 11.x pipeline is indeed green now on the MR, so RTBCing. I looked for 11.4.x on the pipeline daily/weekly jobs, but it is maybe running only on commit? Which might be more resource-efficient anyway given the rate of backports, but earlier release branches have daily or weekly jobs. I did find this 11.4.x job and confirmed the fail there is also:

$ yarn run spellcheck:core --no-progress --no-must-find-files --cache --cache-strategy content
core/lib/Drupal/Core/Archiver/Tar.php:33:58 - Unknown word (lzma)
CSpell: Files checked: 17475, Issues found: 1 in 1 file.

Cleaning up project directory and file based variables


ERROR: Job failed: command terminated with exit code 1

Thus, this should fix 11.4.x as well.

mstrelan’s picture

mstrelan commented

IMHO we should just spellcheck on main and skip for backport branches.

acbramley’s picture

acbramley commented

Ahh didn't realise this issue was still open, I created #3612912: Spellcheck is failing on 11.x for the 11.x failure. Can we get that committed? Either here or in the other issue :)

catch’s picture

catch
he/him
Primary language English
commented
Status: Reviewed & tested by the community » Fixed

Committed/pushed to 11.x and 11.4.x, thanks!

We could probably take a similar approach for cspell to the one in #3580398: Skip on-commit jobs for the main and 11.x branches (in reverse), would save a few thousand CI minutes every year probably.

Now that this issue is closed, review the contribution record.

As a contributor, attribute any organization that helped you, or if you volunteered your own time.

Maintainers, credit people who helped resolve this issue.

  • catch committed d952bfb2 on 11.4.x 
    fix: #3607061 Don't spellcheck composer.json

By: mstrelan
By: quietone...

  • catch committed e74b6b6c on 11.x 
    fix: #3607061 Don't spellcheck composer.json

By: mstrelan
By: quietone...

catch closed merge request !16415

mstrelan’s picture

mstrelan commented

Opened #3612933: Only run cspell on main and MRs that target main as a follow up