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 check-spelling #4017

Closed
wants to merge 41 commits into from
Closed

Add check-spelling #4017

wants to merge 41 commits into from

Conversation

jsoref
Copy link
Contributor

@jsoref jsoref commented Aug 1, 2023

Proposed changes

In #4008 (comment), @ehsandeep suggested adding the check-spelling workflow.

This is the workflow, roughly as I used it to develop the PR, but with a couple of changes.

As configured check-spelling will produce a job summary reachable via Details.

When possible, it will generate a Sarif report, and if a user is an admin (or nearly an admin) they'll be able to view it in GitHub, otherwise, users can download a generated artifact and process w/ their own tools (especially VSCode).

I've configured it to provide a fallback message (using spell_check_this: projectdiscovery/nuclei@dev) (ideally this shouldn't be necessary, but I think I have a bug I need to resolve which I haven't had time to focus on) -- note that this assumes it'll be merged to main before most people notice the workflow.

The advice.md file is a markdown file that the project can customize based on the needs of its maintainers/contributors. The README.md is designed to be helpful documentation, but is absolutely optional.

You'll want to review expect.txt at least once, and potentially periodically (depending on how good people are at paying attention to the tool -- at work people sometimes just accept its suggestions even when it's pointing out obvious typos 🤷‍♂️).

It's using the latest released tag (v0.0.22). Some projects are release-early / release-often -- check-spelling isn't. I don't tend to make a lot of releases although my coworkers are dogfooding prerelease in a dozen repositories (and I'm constantly dogfooding it to make contributions such as the one that was merged here). If you start using GitHub workflows, you'll want to set up dependabot to make PRs for updates to things.

I'm happy to answer questions here / in Discord, and if this is merged, I'll probably watch the repository for a bit to make sure people don't have any problems.

Checklist

  • Pull request is created against the dev branch
  • All checks passed (lint, unit/integration/regression tests etc.) with my changes
  • I have added tests that prove my fix is effective or that my feature works
  • I have added necessary documentation (if appropriate)

@jsoref
Copy link
Contributor Author

jsoref commented Aug 1, 2023

To check this feature, the simplest thing to do is pull this branch into your local repository, create a new branch from it and add a commit with a typo to that branch and push it to your repository, github will automatically run the workflow and report.

To check how it handles PRs, you can create a PR into your branch that has this code from your branch with the typo.

@tarunKoyalwar tarunKoyalwar deleted the branch projectdiscovery:dev August 4, 2023 14:51
@tarunKoyalwar tarunKoyalwar reopened this Aug 4, 2023
Copy link
Member

@ehsandeep ehsandeep left a comment

Choose a reason for hiding this comment

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

Thank you for your assistance, @jsoref! I'm sorry for not getting back to you sooner.

I wanted to check with you on this particular step:

    - name: apply spelling updates
      uses: check-spelling/[email protected]
      with:
        experimental_apply_changes_via_bot: 1
        checkout: true
        ssh_key: "${{ secrets.CHECK_SPELLING }}"

Specifically, could you elaborate on the ssh_key: "${{ secrets.CHECK_SPELLING }}" argument? What value is it expected to hold and is it mandatory for this operation?

@jsoref
Copy link
Contributor Author

jsoref commented Sep 18, 2023

Thanks for asking, -- I've updated my public documentation to try to make it easier for people to understand how this works (sadly the mapping between GitHub Wiki markup and normal Markdown is a mess, so the anchors from the top of the Configuration page won't work....)

https://docs.check-spelling.dev/Configuration.html#sshkey
https://docs.check-spelling.dev/Feature:-Update-with-deploy-key

For this repository, the value of

ssh_key: "${{ secrets.CHECK_SPELLING }}"

will be ignored:
github.repository_owner != 'projectdiscovery' &&

The ssh_key is optional, but when the value is not set, then the GITHUB_TOKEN magical key will be used to publish commits (again, this would only apply to forks, not this repository):

contents: write

secrets.CHECK_SPELLING is designed to be set to a GitHub deploy key with write permission. It enables the published metadata updates to trigger a round of workflow checks, otherwise when someone uses the update metadata feature (on a commit with a ❌ the failed status will be retained.

The general design behind this feature is to make it easier for people using check-spelling to update the metadata when they believe the terms they're adding are temporarily correct (mostly adding items to expect.txt). If you aren't comfortable with this feature, the experimental_apply_changes_via_bot item can be removed from the main job and the update job can be removed entirely.

Signed-off-by: Josh Soref <[email protected]>
Signed-off-by: Josh Soref <[email protected]>
Signed-off-by: Josh Soref <[email protected]>
Signed-off-by: Josh Soref <[email protected]>
Signed-off-by: Josh Soref <[email protected]>
Signed-off-by: Josh Soref <[email protected]>
Signed-off-by: Josh Soref <[email protected]>
Signed-off-by: Josh Soref <[email protected]>
Signed-off-by: Josh Soref <[email protected]>
Signed-off-by: Josh Soref <[email protected]>
Signed-off-by: Josh Soref <[email protected]>
Signed-off-by: Josh Soref <[email protected]>
Signed-off-by: Josh Soref <[email protected]>
Signed-off-by: Josh Soref <[email protected]>
Signed-off-by: Josh Soref <[email protected]>
Signed-off-by: Josh Soref <[email protected]>
Signed-off-by: Josh Soref <[email protected]>
Signed-off-by: Josh Soref <[email protected]>
Signed-off-by: Josh Soref <[email protected]>
Signed-off-by: Josh Soref <[email protected]>
Signed-off-by: Josh Soref <[email protected]>
Signed-off-by: Josh Soref <[email protected]>
Signed-off-by: Josh Soref <[email protected]>
Signed-off-by: Josh Soref <[email protected]>
Signed-off-by: Josh Soref <[email protected]>
Signed-off-by: Josh Soref <[email protected]>
Signed-off-by: Josh Soref <[email protected]>
Signed-off-by: Josh Soref <[email protected]>
Signed-off-by: Josh Soref <[email protected]>
Signed-off-by: Josh Soref <[email protected]>
Signed-off-by: Josh Soref <[email protected]>
Signed-off-by: Josh Soref <[email protected]>
Signed-off-by: Josh Soref <[email protected]>
Signed-off-by: Josh Soref <[email protected]>
Signed-off-by: Josh Soref <[email protected]>
Signed-off-by: Josh Soref <[email protected]>
Signed-off-by: Josh Soref <[email protected]>
Signed-off-by: Josh Soref <[email protected]>
Signed-off-by: Josh Soref <[email protected]>
Signed-off-by: Josh Soref <[email protected]>
@jsoref
Copy link
Contributor Author

jsoref commented Dec 22, 2023

@ehsandeep: don't worry about slow responses. I get it.

Fwiw, this fall I released v0.0.22.

  • I've refreshed the workflow to use it
  • The .github/actions/spelling/candidate.patterns and .github/actions/spelling/line_forbidden.patterns changes are basically improvements in baseline knowledge that I've gathered between when this PR was opened and today.
  • The .github/actions/spelling/excludes.txt changes are because the repository renamed some files (check-spelling automatically suggested adding the new items, although I manually removed the old ones...)
  • In general it's a good idea to review .github/actions/spelling/expect.txt between refreshes as new items are good indications of possible typos -- I think I've gotten all of them in the other commits, but...
  • Items in .github/actions/spelling/patterns.txt that had hit-count: and were removed were no longer suggested (possibly because the files were removed, the content was changed, or excludes applied to them)
  • The changes from cspell1 to cspell in .github/workflows/spelling.yml are because the workflow was originally developed against the dictionaries from what became 0.0.22 and thus the alternate alias was no longer necessary. One additional dictionary was suggested (npm) and it seemed reasonable, so I've included it.
  • As the update job permissions proved to be scary, I've dropped that from this PR.

@jsoref
Copy link
Contributor Author

jsoref commented Dec 22, 2023

Oh, I should note that w/ v0.0.22, there's a pre-generated commit that people can use which I'm hoping will be easy enough to apply the basic suggestions w/o having to worry about running a third party program locally (finding a nice middle-ground for updating metadata has been a hard problem...).

You can see an example of it in https://github.com/check-spelling-sandbox/nuclei/actions/runs/7294645877#summary-19879855114 under To accept these unrecognized words as correct, you could apply this commit.

It's a fairly new feature, so I'm definitely seeking feedback from people who use it, consider/reject using it, etc.

@ehsandeep
Copy link
Member

@jsoref thank you for the assistance and support, we will reconsider this in future once we get more familiar with workflow.

@ehsandeep ehsandeep closed this Jan 5, 2024
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