mermaid icon indicating copy to clipboard operation
mermaid copied to clipboard

Added and configured cspell plugin to eslint

Open devcer opened this issue 3 years ago • 7 comments

:bookmark_tabs: Summary

Added and configured cspell plugin to eslint

Resolves #3601 Resolves #3642 (alternative PR for the same feature)

:straight_ruler: Design Decisions

Describe the way your implementation works or what design decisions you made if applicable.

  • Installed cspell package to package.json
  • Configured the plugin in eslintrc.json
  • Ran pnpm lint to verify its working.

:clipboard: Tasks

Make sure you

  • [x] :book: have read the contribution guidelines
  • [x] :computer: have added unit/e2e tests (if appropriate)
  • [x] :bookmark: targeted develop branch

devcer avatar Oct 08 '22 09:10 devcer

Hi @devcer, thanks for the PR. Working fine.

There are 1358 problems detected in ESLint, out of which 1263 is related to @cspell, which totally obliterates any actual warning we need to see.

There are 2 things we should do.

  1. Integrate the cspell.json added in https://github.com/mermaid-js/mermaid/pull/3600.
  2. Fix new the errors like @SeanKilleen did in #3600 so that this PR doesn't introduce any new warnings.

sidharthv96 avatar Oct 08 '22 11:10 sidharthv96

Hopefully by merging in the develop branch, those things will be resolved. 👍

SeanKilleen avatar Oct 08 '22 11:10 SeanKilleen

Hopefully by merging in the develop branch, those things will be resolved

I believe this branch is already up to date with develop.

The issue is that all of the new warnings are in JavaScript/TypeScript files, which PR #3600 didn't cover (it only checked documentation markdown files).

I tried modifying the spellchecker ESLint rule to ignore identifiers (e.g. variable names), and "strings" and `strings` (basically all that's leftover are comments), and that seems to have cut down the warnings to only 323, and quite a lot look like valid spelling mistakes.

   "@cspell/spellchecker": ["warn", {
      // too many false positives for variable names
      "checkIdentifiers": false,
      "checkStrings": false,
      "checkStringTemplates": false
    }],

aloisklink avatar Oct 08 '22 13:10 aloisklink

Thanks for the response sidharth.

Hi @devcer, thanks for the PR. Working fine.

There are 1358 problems detected in ESLint, out of which 1263 is related to @cspell, which totally obliterates any actual warning we need to see.

There are 2 things we should do.

  1. Integrate the cspell.json added in Automated docs spell-checking via GitHub Actions (and address all reported issues) #3600.

I'm looking into this.

  1. Fix new the errors like @SeanKilleen did in Automated docs spell-checking via GitHub Actions (and address all reported issues) #3600 so that this PR doesn't introduce any new warnings.

Since fixing things here would be a code-level change, I would not recommend fixing all the lint warnings in a single PR. I can fix for a couple of files as an example. There is a higher chance of things getting messed up or broken.

Let me know what you think.

devcer avatar Oct 08 '22 13:10 devcer

I don't think there is a way to point cspell.json for the eslint. I can however configure the following options listed here -> https://www.npmjs.com/package/@cspell/eslint-plugin

Let me know what do you guys think.

devcer avatar Oct 08 '22 13:10 devcer

Since fixing things here would be a code-level change, I would not recommend fixing all the lint warnings in a single PR. I can fix for a couple of files as an example. There is a higher chance of things getting messed up or broken.

Let me know what you think.

If you set the following settings in .eslintrc.json rules field, it should only have warnings in code comments, so there'll be very low-risk of anything getting messed up (from a quick look, code-level warnings are mostly false positives anyway, it seems to me that it only makes sense to check comments):

   "@cspell/spellchecker": ["warn", {
      // too many false positives for variable names
      "checkIdentifiers": false,
      "checkStrings": false,
      "checkStringTemplates": false
    }],

Although, it will probably be a lot of work to fix all the spelling issues (30 minutes to 1 hour or more). You'll probably have to:

  • Fix spelling issue if misspelled
  • Add to ignoreWords if false positive (e.g. for things like dagre, which is the name of a library)
  • Fix camelCase capitalization for things that should be multiple words, (e.g. mymermaid should be myMermaid for cspell to realize it's two words)

If it's too much work, we can just leave this PR open for somebody else to work on it. :smile:

I don't think there is a way to point cspell.json for the eslint.

My gut feeling is that cSpell.json is automatically loaded, but I can't find any documentation confirming that.

aloisklink avatar Oct 09 '22 17:10 aloisklink

@aloisklink I have added the rules specified to .eslintrc.json, configured all the ignore words from cspell.json, and also fixed the lint errors in a few files. these were mostly comments so I'm confident that nothing would break on the functionality side of things.

Let me know if there is anything else to be done here.

devcer avatar Oct 11 '22 17:10 devcer

@aloisklink Thanks for your commit. I have removed a couple of more eslint-ignore statements and added related words to cspell.json. I see 0 eslint warnings related to spellings.

devcer avatar Oct 17 '22 04:10 devcer