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.

Sign up for GitHub

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

Jump to bottom

Tune check-spelling #203

Merged
merged 1 commit into from
Sep 28, 2023
Merged

Tune check-spelling #203

merged 1 commit into from
Sep 28, 2023

Conversation

jsoref
Copy link
Contributor

@jsoref jsoref commented Sep 20, 2023

This is an alternative to #201 as proposed in #201 (comment)

  • Add commenting on PRs
  • Add commenting on push for repositories not in ossf
  • Disable failure status reporting
  • Only check changed files
    • Disable sarif reporting
    • Remove update job
  • Adjust advice

With these changes, you'll get a comment like this: check-spelling-sandbox#3 (comment) and no ❌.

@jsoref
Tune check-spelling
f9c54b6 
* Add commenting on PRs
* Add commenting on push for repositories not in ossf
* Disable failure status reporting
* Only check changed files
  * Disable sarif reporting
  * Remove update job
* Adjust advice

Signed-off-by: Josh Soref <2119212+jsoref@users.noreply.github.com>
steiza
steiza approved these changes Sep 25, 2023
View reviewed changes
Copy link
Member

@steiza steiza left a comment

Choose a reason for hiding this comment

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

I think it's nice to have the spelling feedback without blocking.

mlieberman85
mlieberman85 approved these changes Sep 27, 2023
View reviewed changes
Copy link
Contributor

@mlieberman85 mlieberman85 left a comment

Choose a reason for hiding this comment

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

These all seem reasonable!

lehors
lehors approved these changes Sep 28, 2023
View reviewed changes
Copy link
Contributor

@lehors lehors left a comment

Choose a reason for hiding this comment

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

Ok, this sounds like a reasonable compromise. Let's try it.

@lehors lehors merged commit 993de76 into ossf:main Sep 28, 2023
3 checks passed
@lehors lehors mentioned this pull request Sep 28, 2023
Merged
@jsoref jsoref deleted the tune-check-spelling branch September 28, 2023 11:47
@lehors
Copy link
Contributor

lehors commented Sep 28, 2023 •
edited
Loading

@jsoref I'm sorry but I don't understand how to parse this sentence. "To accept ✔️ these unrecognized words as correct and remove the previously acknowledged and now absent words, run the following commands".
First, I think the ✔️ symbol doesn't help, instead it breaks the flow of the sentence. But then I really struggle with that part: "remove the previously acknowledged and now absent words". When were words previously acknowledged? Why are they absent now?
I'm confused.

@jsoref
Copy link
Contributor Author

jsoref commented Sep 28, 2023

First thanks for your feedback, it really helps a lot.

The ✔️s are something I added in v0.0.20 (current is v0.0.21, although I was hoping to ship a v0.0.22 today...) -- check-spelling/check-spelling@ccfe971 to try to make workflows easier to identify/discover.

Words in expect.txt (or similar) were accepted by previous commits.
They can be absent for a number of reasons:

  1. they could have been removed
  2. they could no longer be checked (excludes.txt / only.txt)
  3. they could be masked (patterns.txt)
  4. they could be in the dictionary (extra_dictionaries / allow.txt)
  5. someone could have accidentally added a word to expect.txt that didn't belong there

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@lehors lehors lehors approved these changes

@mlieberman85 mlieberman85 mlieberman85 approved these changes

@steiza steiza steiza approved these changes

Assignees
No one assigned
Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
4 participants
@jsoref @lehors @steiza @mlieberman85