Skip to content
New issue

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

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

Already on GitHub? # to your account

Add check-spelling action #94

Merged
merged 2 commits into from
Oct 7, 2023
Merged

Conversation

jsoref
Copy link
Contributor

@jsoref jsoref commented Oct 1, 2023

In #92 (comment), @Nishchit14 asked about 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: firecamp-dev/firecamp@main) (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).

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 🤷‍♂️). Upon reviewing it while drafting this PR, I noticed a couple of typos (and that I'd effectively missed that in my previous runs).

Notably, Ramanujam and ramanujan below are suspiciously similar (and both are commented out):

packages/firecamp-scripts/src/fc/response/assertions.ts:            .and.have.jsonBody('name', 'Ramanujam')
platform/firecamp-platform/src/store/environment.ts:    //       value: 'ramanujan',

-- I haven't done anything about this, as I'm uncertain about which is likely to be correct or whether they're definitely related (although proximity leads me to be believe that they are).

The update job is designed to only run in forks (see check-spelling-sandbox#3 for an example of it refusing to run in the configured repository). It enables people to talk to their own instance instead of running the update command locally / manually editing expect.txt / excludes.txt. You're encouraged to play with this logic in a fork, or if you aren't comfortable with it, there's nothing wrong with omitting the job.

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). This workflow includes a brand new workflow tweak to make future dependabot driven upgrades slightly smoother (see Suppress PR check when workflow changes for details).

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.

@netlify
Copy link

netlify bot commented Oct 1, 2023

‼️ Deploy request for staging-firecamp-dev rejected.

Name Link
🔨 Latest commit 7f561e3

@Nishchit14
Copy link
Contributor

thank you @jsoref for the PR. i have seen that this is adding too many commits. is it possible to combine all spell checks as single/long commit message under only one commit? i'll be back on routine in two days and then deep dive more into the PR and configurations.

jsoref added 2 commits October 3, 2023 07:57
* atlassian
* divider
* foreground
* items
* managed
* offset
* request
* response
* secondary
* want
* workspace

Signed-off-by: Josh Soref <2119212+jsoref@users.noreply.github.com>
@jsoref
Copy link
Contributor Author

jsoref commented Oct 3, 2023

@Nishchit14: I've collapsed the spelling fixes.

Copy link
Contributor

@Nishchit14 Nishchit14 left a comment

Choose a reason for hiding this comment

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

LGTM :)

@Nishchit14
Copy link
Contributor

The PR looks awesome!! Thanks @jsoref. However, the action configuration seems to be a little complex for first-time contributors. but I am happy to try it for some time to adopt it. Here I have two points to share

  1. Do we need to add the doc related to spelling-chek action for Firecamp Contributors? In CONTRIBUTING.md
  2. This PR is created as DRAFT, Is this ready for merge?

@jsoref
Copy link
Contributor Author

jsoref commented Oct 6, 2023

Hmm, yeah, you might want to put some text into CONTRIBUTING.md, sorry, I haven't started thinking about that part - I definitely don't have boilerplate for doing that. The general idea is that if you get an ❌, you can go to details for the check and view the job summary and it should explain what it found and offer suggestions about what to do... Generally,

  • if words are misspelled, they should be corrected,
  • if words aren't in the dictionary and should be, either a dictionary can be added (which will not take effect for the purpose of the pull request, but will going forward) or they can be added to allow.txt (which functions as a supplemental dictionary)
  • if files shouldn't be checked, regular expressions to match them can be added to excludes.txt,
  • if patterns within a single line should be skipped, they can be added to patterns.txt
  • beyond that, usually items will be added to or removed from expect.txt - check-spelling is somewhat case-sensitive -- an entry in expect.txt needs to correspond to text discovered in the repository, but if a lowercase item is found and present, it will also cover a Title variant (the understanding being that it could be the first word in a sentence), and for either a lowercase or Title case word it'll cover a CAPITALIZED variant (the expectation being that's used in certain instances), but the reverse doesn't apply, this enables flagging, of, e.g. a brand that always wants to be CAPITALIZED or wants to use Title case and not lowercase.

It might be sufficient to link to the readme I've included or to https://docs.check-spelling.dev.

(I might take some of the text above and copy it there...)

From my perspective, the PR is ready, everything else is tunable later.

@jsoref jsoref marked this pull request as ready for review October 6, 2023 11:46
@Nishchit14
Copy link
Contributor

It's reasonable, I am merging this PR. Huge thanks for it.

@Nishchit14 Nishchit14 merged commit d43e50d into firecamp-dev:main Oct 7, 2023
@jsoref jsoref deleted the spell-check branch October 8, 2023 12:21
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants