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 #682

Merged
merged 119 commits into from
Feb 3, 2023
Merged

Add CSpell #682

merged 119 commits into from
Feb 3, 2023

Conversation

kevaundray
Copy link
Contributor

@kevaundray kevaundray commented Jan 21, 2023

Related issue(s)

Resolves #681

Description

Summary of changes

Dependency additions / changes

Test additions / changes

Checklist

  • I have tested the changes locally.
  • I have formatted the changes with Prettier and/or cargo fmt with default settings.
  • I have linked this PR to the issue(s) that it resolves.
  • I have reviewed the changes on GitHub, line by line.
  • I have ensured all changes are covered in the description.

Additional context

@kevaundray kevaundray changed the title Kw/add cspell Add CSpell Jan 21, 2023
@kevaundray
Copy link
Contributor Author

Note that the json file also includes words which are misspelt -- This is intentional and once they have been removed and corrected, the PR will be ready for review

@kevaundray kevaundray marked this pull request as draft January 21, 2023 16:50
@kevaundray kevaundray marked this pull request as ready for review January 21, 2023 16:55
@kevaundray kevaundray closed this Jan 21, 2023
@kevaundray kevaundray reopened this Jan 21, 2023
- A simple test to check that the CI fails when an unknown word is in the code
@kevaundray
Copy link
Contributor Author

To print out all of the unknown words, one can install the cli and run:

cspell-cli "crates/**/*.{md,rs}" --words-only --no-summary --no-progress --unique

@kevaundray
Copy link
Contributor Author

A large group of the spelling mistakes fall into two categories:

  • Mispelled words (Seems to be about half of the list)
  • incorrect casing; for example myfunc is seen as one word, but my_func or myFunc will be recognised as two words

Then we have domain specific words like acvm, acir and rustc which should indeed be added to the list.

@kevaundray
Copy link
Contributor Author

In the worse case, if we keep adding new words to the codebase one will need to keep updating the list.

The upside is that we no longer need to manually check for spellling mistakes from each contributor and the introduction of a new word will immediately be obvious (warranting an explanation)

Copy link
Contributor

@jfecher jfecher left a comment

Choose a reason for hiding this comment

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

Does this PR fix any issues with the codebase? I'm somewhat dubious of the need for a spellchecker. I don't see it solving any issues and it just adds an extra CI step.

cspell.json Outdated Show resolved Hide resolved
@kevaundray
Copy link
Contributor Author

Does this PR fix any issues with the codebase? I'm somewhat dubious of the need for a spellchecker. I don't see it solving any issues and it just adds an extra CI step.

We do have quite a lot of spelling mistakes -- if you check the cspell.json file, I have added the words which were misspelled into the json file to indicate them and to have the CI pass.

The top ones are spelling mistakes and in other places, it is because we are not using the snake case to correctly separate two words for example.

Getting it in the CI is a pain -- we can also opt to make them warnings in the PR instead of errors, though I don't expect us to add a bunch of new unknown words, so I'm up for trying it out and seeing how often it catches new spelling mistakes introduced by a PR

@jfecher
Copy link
Contributor

jfecher commented Jan 23, 2023

If it is not too much trouble then, I think this PR should also fix the spelling mistakes it finds rather than committing an ignore file with many mistakes we do not want ignored. If it is too much work, you can pass it off to someone else on the team.

@kevaundray
Copy link
Contributor Author

If it is not too much trouble then, I think this PR should also fix the spelling mistakes it finds rather than committing an ignore file with many mistakes we do not want ignored. If it is too much work, you can pass it off to someone else on the team.

I agree -- to clarify, this PR is not ready while we are ignoring spelling mistakes. This was just done so that the CI can pass and we are aware of the list of problematic words.

I then removed a word from the list to check that the CI failed and now it's a matter of correcting the spelling mistakes present in the code and removing them from the Json file. I will do this and then re-request your review :)

Copy link
Contributor

@jfecher jfecher left a comment

Choose a reason for hiding this comment

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

👍

@kevaundray kevaundray merged commit 126ca26 into master Feb 3, 2023
@kevaundray kevaundray deleted the kw/add-cspell branch February 3, 2023 19:31
TomAFrench added a commit to TomAFrench/noir that referenced this pull request Feb 3, 2023
* master:
  chore: Add spellchecker  (noir-lang#682)
  Rename methods that use `conditionalize` to be more descriptive (noir-lang#739)
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.

Add Cspell
2 participants