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

Use check-spelling/check-spelling@v0.0.20 #97

Merged
merged 1 commit into from
Aug 22, 2022

Conversation

jsoref
Copy link
Contributor

@jsoref jsoref commented Aug 22, 2022

Originating Project/Creator
Affected Component
Affected Architectures(s)
Related Issue(s)
Has Unit Tests (y/n)
Builds Without Errors (y/n)
Unit Tests Pass (y/n)
Documentation Included (y/n)

Change Description

Refreshes the workflow based on
https://github.com/check-spelling/spell-check-this/blob/744c66e2140fd8acaf5388efd0db3727d010d6e9/.github/workflows/spelling.yml

Rationale

  • Updates to the latest workflow (including permissions separation)
  • Updates to current action version
  • Fixes -D pattern

Testing/Review Recommendations

Fill in testing procedures, specific items to focus on for review, or other info to help the team verify these changes are flight-quality.

You can create PRs into https://github.com/check-spelling/fprime-tools if you'd like, it's running this version.
Runs were here: https://github.com/check-spelling/fprime-tools/actions/workflows/spelling.yml

I've moved a bunch of items from expect.txt to allow.txt which makes it easier to transition from v0.0.19 to v0.0.20, note that this update changes the minimum word length from 2 to 3. Once this is merged, a future PR by someone will remove the 2 letter words from expect.txt.

Future Work

There will be future updates to check-spelling. I'm starting to look into SARIF reporting, however I don't have an ETA for it.

runs-on: ubuntu-latest
needs: spelling
permissions:
contents: write
Copy link
Contributor

@Joshua-Anderson Joshua-Anderson Aug 22, 2022

Choose a reason for hiding this comment

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

Why does the spelling action need contents write permissions?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

GitHub treats commenting on a commit as a write operation (on the repository contents). And commenting on a pul request as a write operation (on pull requests).

It's an unfortunate detail.

But note that the default permissions on most organizations is roughly *: write, so this is actually a reduction in the scope permissions of the workflow.

@Joshua-Anderson
Copy link
Contributor

Thanks so much @jsoref. I was looking at the documentation and trying to understand the nuance about the differences between the allowlist and the expectlist, but it didn't seem to be covered. What is the difference between the two lists?

@jsoref
Copy link
Contributor Author

jsoref commented Aug 22, 2022

(Ouch, I just lost my comment.)

So, I can add this file to the PR (I sometimes do when refreshing, but apparently didn't here):
https://github.com/check-spelling/spell-check-this/blob/main/.github/actions/spelling/README.md -- ideally it'd be clear enough on its own.

My current thoughts on improving this in general are here: https://github.com/check-spelling/check-spelling/wiki/Feature%3A-Easier-to-understand-filenames

But in short, allow.txt (and extra_dictionaries) enable the repository to add additional words to the dictionary (and reject.txt allows removing words from the dictionary).

As check-spelling runs, it compares tokens it finds against its dictionary. Any items not found are then compared against expect.txt. If it finds an item that isn't in expect.txt it screams ❌. If there are things in expect.txt that it didn't find, it will suggest removing them.

Imagine you had a misspelled item in the repository, it'd be in expect.txt, if you add a commit to fix the misspelling, then unless the item is removed from expect.txt, if someone later goes to add the same misspelling, check-spelling wouldn't know to complain again. So, by automatically suggesting pruning operations to expect.txt, check-spelling ensures that corrected typos aren't later reintroduced accidentally.

Copy link
Contributor

@Joshua-Anderson Joshua-Anderson left a comment

Choose a reason for hiding this comment

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

Thanks again @jsoref for helping us with the upgrade. I'll take a stab at updating the other F' repositories using check-spelling later today.

@Joshua-Anderson Joshua-Anderson merged commit 01953bd into nasa:devel Aug 22, 2022
@jsoref jsoref deleted the spell-check branch August 22, 2022 20:11
@jsoref
Copy link
Contributor Author

jsoref commented Aug 22, 2022

Note that because of the way github workflows work, you should upgrade all branches.

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.

None yet

2 participants