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

chore: Allow spellchecking to fail the build #1497

Closed
wants to merge 1 commit into from

Conversation

nschonni
Copy link
Contributor

This landed awhile ago with it being allowed to fail. I think it's been around awhile and the instances where is started to fail PRs were valid rather than false positives, so I think it's OK to enable this now to fail PRs

Copy link
Contributor

@carmacleod carmacleod left a comment

Choose a reason for hiding this comment

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

Happy to have spellcheck fail builds. :)

@mcking65
Copy link
Contributor

mcking65 commented Aug 20, 2020

Not sure we want this ... will add one more complexity to merging, and we already struggle with the weight of current complexities. If spellcheck is informative, then we won't have multiple PRs modifying the exceptions. We can instead occasionally review/modify exceptions and make changes in batches.

One thing we should do as part of editorial review is look at the result of spellcheck test and see if any of the failures are due to mistakes in the PR being reviewed. If the PR contains content that requires a spelling exception, I'd rather not be forced to make that change to speellcheck in that PR.

Net, it doesn't have to be a required check to be useful.

What are some counter arguments?

@nschonni
Copy link
Contributor Author

I'm going to close this one in favour of #1520

@nschonni nschonni closed this Sep 25, 2020
@nschonni nschonni deleted the patch-2 branch September 25, 2020 19:57
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