Disable pedantic spellcheck in CI

[fredlas]

check_spelling is already catching common misspellings. check_spelling_pedantic is making us update a dictionary every time we write an unusual and/or technical word in a comment.

[htuch]

@envoyproxy/maintainers thoughts?

[mattklein123]

I don't know the history in terms of what check_spelling does vs. check_spelling_pedantic. IIRC, check_spelling did not check a huge amount of things that the pedantic check fixed. My vote would be to keep this and just update the pre-commit hooks to run the spelling check also on the files being submitted. I can take a look at this tomorrow as I've been working on the tooling stuff for a related project recently.

[moderation]

The pre-commit or push hook is what @mergeconflict suggested in Slack at https://envoyproxy.slack.com/archives/C78HA81DH/p1555095788013400. I'd vote to keep the check too and look at this hook approach. As @mergeconflict suggests it might be possible to catch the errors pre CI.

[fredlas]

The problem with the git hook approach is that it doesn't actually save any work, it just speeds up the process of realizing that the work needs to be done. I certainly would vote for it over keeping things as is, but I thing getting rid of it altogether is best.

The real thing to consider here is the cost+benefit: it's just some typos, and only uncommon ones at that, due to the regular spell_check. Comprehension being messed with due to poor writing is a real thing, but 99% of that is going to be coming from poor/awkward grammar, not typos. I maen, tehre is taht pyschlogoiacl pheonmneon wehre you olny actlauly raed the frist and lsat lteters, right? Given that, I think any amount of work to get perfect spelling isn't worth it.

[htuch]

My $0.02, FWIW. I'd definitely be in favor of strict spell checking on documentation which is user facing; this includes anythings in docs/, api/ and even include/envoy/ (one day we will generate Doxygen, real soon now ;) ). Envoy's great documentation is one of its key differentiators and having high quality docs (which includes great spelling and grammar) is part of this.

Source comments don't bother me as much and I'm in agreement with @fredlas on those.

@zuercher do you want to chime in, given this was introduced in your PR?

[fredlas]

That's a really good point. I would mind it much much less in there. The two sources of false positives seem to be code things like "CMSG", and sort of unusual words like "unwatched", both of which are way less likely to show up in clean documentation talking about high level stuff, rather than weird implementation details.

[mattklein123]

I think the effort of seeing a spelling error and adding a fix to the dictionary is being greatly exaggerated. IT takes 30 seconds.

[zuercher]

I think it's useful to spell check the comments. I believe it helps with readability (even if grammar is still an issue). I also don't find it takes much extra time, but I'll concede I'm a reasonably good speller to begin with and my editor is configured to spell check comments.

FWIW, I called the new spell checker "pedantic" mostly to distinguish it from the original check_spelling.sh, which only looks for a specific list of misspellings. Specifically, it detects words in this list, which hasn't been updated since June 2017. I planned to eventually remove the original script.

I think having the spell check run as a git commit hook would be useful, although it introduces more dependencies on build environments.

Some other ideas that might be worth pursuing:

• Perhaps words in all caps should be treated as warnings vs. errors when misspelled. There are certainly compiler macros referenced in comments that end up in the dictionary (e.g. EINVALID). Spell checking them in comments guarantees you can find the comments when searching for the macro, but that might not be sufficient reason to continue.
• It may be possible configure the dictionary to allow, for example, "un-" as a prefix on any correctly spelled word. I'll look into this.

[fredlas]

The issue isn't with real misspellings (which it has caught only one of for me so far); how good your spelling is isn't a factor. The problem is adding false positives to the dictionary. Those improvements sound like they would definitely help with the false positives. Maybe disable temporarily until they're ready?

Regarding the time it wastes: 30 seconds adds up. If my experience is typical, let's say 1 per couple thousand lines added, this has probably wasted like an hour or more so far. Also, 30 seconds is a good estimate for someone who is familiar with this setup. The first time I ran into this, it took me several minutes to figure out what was going on.

[mattklein123]

@fredlas @zuercher I opened #6631 to run the pedantic check in the pre-push hook. IMO this makes the feedback cycle tight enough that it should be pretty easy to deal with. I think independently we can look into some of the improvements that @zuercher mentions but IMO we don't need to disable this check in CI.

[fredlas]

Alright, looks like the consensus is no.