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

🔥 remove spelling workflow/action #201

Merged
merged 1 commit into from
Jul 17, 2024
Merged

Conversation

ctcpip
Copy link
Member

@ctcpip ctcpip commented Sep 20, 2023

@jsoref
Copy link
Contributor

jsoref commented Sep 20, 2023

Is there any feedback you have about the experience?

@ctcpip
Copy link
Member Author

ctcpip commented Sep 20, 2023

spell checkers are great extensions to use on one's editor locally, but not something good to have as a status check on repositories

@jsoref
Copy link
Contributor

jsoref commented Sep 20, 2023

ok, thanks for trying it. I appreciate it.

@ctcpip
Copy link
Member Author

ctcpip commented Sep 20, 2023

to elaborate:

  • most spelling problems found are false positives
  • nobody wants to see a red ❌ on a commit, especially for a spelling problem, and moreso when it's a false positive
  • it is burdensome to constantly be updating configuration files / dictionaries
  • doing the above increases surface area for false negatives

it would be more tolerable if it wasn't a status check, and instead did something like add a comment on the pull request that points out the spelling issues and line numbers (on newly added content only). these comments could be addressed, or ignored if false positives, and it avoids most of the problematic issues with having it be a status check.

@jsoref
Copy link
Contributor

jsoref commented Sep 20, 2023

  • It's possible to configure it as a comment on PRs (although that means it needs pull-requests: write, and some people don't like that)
  • It's possible to have it not yield a ❌ -- quit_without_error: true
  • only checking the changes for the PR, is also possible: only_check_changed_files: true although it is scoped to the file, as opposed to the patch hunks

Would you like to try that configuration?

@hythloda hythloda self-requested a review September 20, 2023 16:28
Copy link
Member

@hythloda hythloda left a comment

Choose a reason for hiding this comment

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

Nice change!

@jsoref
Copy link
Contributor

jsoref commented Sep 20, 2023

Here's a demo check-spelling-sandbox#3 (comment) of a configuration check-spelling-sandbox@ea13b38 as described in #201 (comment)

There are typos included in the PR base check-spelling-sandbox@b8154f4, but they aren't reported in the PR.

If that's good enough, I can make a PR for it.

(Note that check-spelling doesn't have the ability to check the just the hunks that are added, although as it seems there's interest, I might prioritize that for the release after the one I'm about to ship.)

@jsoref jsoref mentioned this pull request Sep 20, 2023
@lehors
Copy link
Contributor

lehors commented Sep 28, 2023

I'm closing this for now while we try PR #203 which seems to address the main issue. We can reopen it later on if the experiment doesn't work.

@lehors lehors closed this Sep 28, 2023
@lehors
Copy link
Contributor

lehors commented Jul 17, 2024

Having tested this for quite a while now it's become clear that while this checker catches a few typos here and there the number of false positive is so high that it is primarily a nuisance. It seems time to take it off this repo.

@lehors lehors reopened this Jul 17, 2024
@lehors lehors requested a review from a team as a code owner July 17, 2024 16:03
Signed-off-by: ctcpip <ctcpip@users.noreply.github.com>
@lehors lehors force-pushed the remove-workflow branch from 32febc9 to 556ce78 Compare July 17, 2024 16:21
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.

Let's do it.

@bobcallaway
Copy link
Contributor

Looks gewd to me

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.

Lgtm

@SecurityCRob SecurityCRob merged commit 914f2e9 into ossf:main Jul 17, 2024
4 checks passed
@ctcpip ctcpip deleted the remove-workflow branch July 17, 2024 18:40
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.

7 participants