Problem/Motivation

Some of the words are positioned in not alphabetical order.

Proposed resolution

Add test for sorting order of dictionary.txt file

Remaining tasks

Add test
Fix list sorting

Issue fork drupal-3336828

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:

Comments

voleger created an issue. See original summary.

voleger’s picture

Assigned: voleger » Unassigned
Status: Needs work » Needs review

CSpell/SortTest shows that the file contains a list that is not sorted correctly.

Adding changes to the list makes the test passing.

voleger’s picture

Issue tags: +Quick fix
longwave’s picture

Status: Needs review » Needs work

The dictionary generator is supposed to sort the file, but the error proves that we must have merged some changes manually; so the test is good to ensure that it stays sorted.

A few nitpicks in the test but overall this looks great.

spokje’s picture

The dictionary generator is supposed to sort the file, but the error proves that we must have merged some changes manually;

Aren't we fighting the symptom here?

Maybe we can trigger a yarn spellcheck:make-drupal-dict in commit-code-check.sh on all files if dictionary.txt changes?
If the the new dictionary is different from the one in the patch/MR then fail and we should be fine?

longwave’s picture

yarn spellcheck:make-drupal-dict is quite slow, this test should be quick. I also think there are still some locale issues with the dictionary generator, as the accented characters sometimes change for reasons I don't yet understand, and regenerating on DrupalCI might trip this up further.

longwave’s picture

Having just written #7 it strikes me that the issue might be that we need to apply LC_ALL=C to the tr command as well as the sort command.

voleger’s picture

Status: Needs work » Needs review

Thanks for the review, I addressed all review comments.

longwave’s picture

Status: Needs review » Reviewed & tested by the community

Thanks, this looks OK to me and will prevent regressions here in the future.

voleger’s picture

voleger’s picture

catch’s picture

Status: Reviewed & tested by the community » Fixed

I was tempted to agree with Spokje that we could skip this step and go straight to #3338155: Run CSpell on all files when CSpell-related files change... but cherry-picks bypass core commit checks, so this should catch situations where we're introducing discrepancies between 10.1.x and 10.0.x (although only post commit so if it happens we'll need to hotfix it).

Committed/pushed to 10.1.x and cherry-picked to 10.0.x, thanks!

  • catch committed 1fc304e5 on 10.0.x
    Issue #3336828 by voleger, longwave, Spokje: File dictionary.txt is not...

  • catch committed 78de9023 on 10.1.x
    Issue #3336828 by voleger, longwave, Spokje: File dictionary.txt is not...

Status: Fixed » Closed (fixed)

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

jonathan1055’s picture

If all the commits are done, can the MR be closed as it was not actually merged here.