Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add cspell dictionary #11953

Open
wants to merge 5 commits into
base: main
Choose a base branch
from
Open

Add cspell dictionary #11953

wants to merge 5 commits into from

Conversation

jjspace
Copy link
Contributor

@jjspace jjspace commented Apr 23, 2024

Description

These are some small repo changes that I've had locally for a little while and on't want to keep git-ignoring locally or stashing every PR

  1. Add Code spell checker as a workspace recommended extension
  2. Add .vscode/cspell.json to include a dictionary of words used in this project for the spell checker
  3. Add @ts-expect-error in CesiumSandcastle because it's using the "wrong" require function and if you have JS Type checking on it flags the entire file
    2024-04-23_11-52

Code Spell Checker

Code spell checker is really helpful when coding to make sure variable names and strings are spelled correctly. Longer term we may want to add cspell as a CI or pre-commit check but right now there's too many words that would be flagged as incorrect to justify that effort. To that end I do want to push people to use the extension more and collectively, gradually build up the dictionary until we're at a point there are very few flagged words and using the cli is more seemless.

When a word is flagged you just have to put the cursor on it and trigger the VSCode suggestion window Ctrl+. for me. Then just make sure you select the cspell.json file to make sure it's added to the repo's dictionary instead of your user or workspace settings.

2024-04-23_11-54
2024-04-23_11-54_1

@ggetz I know we discussed this and I think you have a list of words already in your user settings, feel free to copy those over and commit to this branch if you have a bunch to add intantly

Issue number and link

Part of #11954

Testing plan

Not much to test.
You can try turning on JS type checking for the sandcastle if you want, just be aware there are still lots of other TS errors we have in the library.
Add the extension if you don't already have it and flag a word and make sure you understand how to add words to our dictionary.

Author checklist

  • I have submitted a Contributor License Agreement
  • I have added my name to CONTRIBUTORS.md
  • I have updated CHANGES.md with a short summary of my change
  • I have added or updated unit tests to ensure consistent code coverage
  • I have update the inline documentation, and included code examples where relevant
  • I have performed a self-review of my code

@jjspace jjspace requested a review from ggetz April 23, 2024 16:03
Copy link

Thank you for the pull request, @jjspace!

✅ We can confirm we have a CLA on file for you.

@jjspace jjspace mentioned this pull request Apr 23, 2024
@ggetz
Copy link
Contributor

ggetz commented Sep 20, 2024

@lukemckinstry Could you please review and work with @jjspace to get this over the finish line?

@jjspace
Copy link
Contributor Author

jjspace commented Sep 20, 2024

I just pushed a couple more commits to add some more configuration and a command to actually run the CLI checker using npm run cspell. I did some small spelling fixes but I don't think it's worth trying to address them all at once. These should be fixed as people notice them in other PRs and eventually we will probably be able to set up automated checking with CI.

For our repo it gets a little tricky because we use a lot of acronyms and geospatial specific language that is not recognized. We can add more dictionaries to separate these into specific files if we want. I started with this for names but then realized it could be overkill so we should probably add them only as needed.

@lukemckinstry
Copy link
Contributor

Looks good and works as intended. Currently npm run cspell generates >2400 warnings, which as you point out are mostly valid misspellings and/or technical terms. I agree we can do corrections or add words to the ignore list gradually over time after committing this PR.

This is such a big code base with so many words that I do have a slight worry we could have collisions between valid jargon we want to ignore versus spelling mistakes we want to catch. I guess the solution there is to be mindful not to assign variable/class names that are/resemble misspellings.

  1. any plans to add this to the Contributor Guide docs?
  2. is there a way to run this spell checker on certain files, such as files modified in a PR? That could be good to include in the docs as recommended practice.
  3. Are you still working on other areas of this PR? If not, then once you answer 1 & 2 above I'm happy to approve.

@jjspace
Copy link
Contributor Author

jjspace commented Sep 24, 2024

@lukemckinstry I added a little bit to the docs to point it out.

Copy link
Contributor

@lukemckinstry lukemckinstry left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for adding docs. A few small items

  • lets add Spelling with an anchor link to the table of contents
  • I think the docs should say npm run ... , which in turn point to the script in package.json which uses npx ...

- Run `npm run cspell` to check all files
- Run `npx cspell -c .vscode/cspell.json [file path]` to check a specific file
- If you are using VSCode you can use the [Code Spell Checker](https://marketplace.visualstudio.com/items?itemName=streetsidesoftware.code-spell-checker) extension to highlight misspelled words and add them to our wordlist if they are valid.
- Using cspell optional while we build up the wordlist but may eventually be required as part of our git hooks and CI. See [this issue](https://github.com/CesiumGS/cesium/issues/11954) for an active status on that.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
- Using cspell optional while we build up the wordlist but may eventually be required as part of our git hooks and CI. See [this issue](https://github.com/CesiumGS/cesium/issues/11954) for an active status on that.
- Using cspell is optional while we build up the wordlist but may eventually be required as part of our git hooks and CI. See [this issue](https://github.com/CesiumGS/cesium/issues/11954) for an active status on that.


- We have a basic setup for `cspell` to spellcheck our files. This is currently not enforced but recommended to use and check while programming. This is especially true for JSDoc comments that well end up in our documentation or Readme files
- Run `npm run cspell` to check all files
- Run `npx cspell -c .vscode/cspell.json [file path]` to check a specific file
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
- Run `npx cspell -c .vscode/cspell.json [file path]` to check a specific file
- Run `npm run cspell -c .vscode/cspell.json [file path]` to check a specific file

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants