Skip to content

Add codespell support (config, workflow to detect/not fix) and make it fix few typos #6872

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

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

yarikoptic
Copy link

Got inspired by

❯ git log origin/master | grep typo | nl
     1	    Fix typo in TLS group x25519mlkem768
...
     6	    I'm so tired of typos
...
  44	    Fix README typo, sigh...

although currently there is pretty much no typos left (good job!). With codespell -- it could stay like that.

More about codespell: https://github.com/codespell-project/codespell .

I personally introduced it to dozens if not hundreds of projects already and so far only positive feedback.

CI workflow has 'permissions' set only to 'read' so also should be safe.

@CLAassistant
Copy link

CLAassistant commented Mar 4, 2025

CLA assistant check
All committers have signed the CLA.

Copy link
Member

@mholt mholt left a comment

Choose a reason for hiding this comment

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

Thanks; but looking at the changes I have mixed feelings... some of the "misspellings" are valid, a couple are intentionally whimsical, and the others are mainly just nits about hyphens.

There's a few corrections here I would accept but I think most of these I want us to leave as-is; and I'm not sure that an extra CI job is going to be worthwhile since we'd be littering codespell:ignore in many places.

I think I'm more likely to merge this without the CI job, and with only a few of the typos corrected.

…agically

=== Do not change lines below ===
{
 "chain": [],
 "cmd": "codespell -w",
 "exit": 0,
 "extra_inputs": [],
 "inputs": [],
 "outputs": [],
 "pwd": "."
}
^^^ Do not change lines above ^^^
@yarikoptic
Copy link
Author

Thanks; but looking at the changes I have mixed feelings... some of the "misspellings" are valid, a couple are intentionally whimsical, and the others are mainly just nits about hyphens.

There's a few corrections here I would accept but I think most of these I want us to leave as-is; and I'm not sure that an extra CI job is going to be worthwhile since we'd be littering codespell:ignore in many places.

I have added those words to be treated "legit" directly in the added .codespellrc file. codespell:ignore are preferable (IMHO) only for one-off use cases (some regexes, unique fun, etc). As you can see, given the history/size of the codebase, and some idiosyncrasies (config vars correspondence) -- codespell opinion aligned greatly with the "language" used in caddy -- only a handful of skips to be done. But as I pointed out, git history points to many instances of typo fixes which likely could be avoided if such CI was set up.

I think I'm more likely to merge this without the CI job, and with only a few of the typos corrected.

I will leave it for your final decision -- easy for me to drop it now or for you later if it does become a nuisance. Doesn't matter to me frankly -- but I did have a handful of encounters where CI was not added, and me coming back in a year or so with more fixes simply through running codespell since otherwise folks didn't run/detect it. If you used pre-commit -- alternative could have been just to add it at that level.

Copy link
Member

@mholt mholt left a comment

Choose a reason for hiding this comment

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

Looking more acceptable now -- but a question about the config file.

Is it possible to move that .codespellrc file into .github or something? I've been hoping to un-pollute our project root.

check-hidden = true
# ignore-regex =
# "idiosyncracies" or reflections of some config values like pass_thru
ignore-words-list = ot,te,ond,fo,tread,noes,pass-thru,pass-thrus,keep-alives
Copy link
Member

Choose a reason for hiding this comment

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

What are ot, te, ond, fo, and why is tread on there? (Tread is a dictionary word.)

Copy link
Author

Choose a reason for hiding this comment

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

an example of hits for `ot`
❯ git grep -i '\<ot\>'
.codespellrc:ignore-words-list = ot,te,ond,fo,tread,noes,pass-thru,pass-thrus,keep-alives
go.mod:     go.opentelemetry.io/contrib/propagators/ot v1.17.0 // indirect
go.sum:go.opentelemetry.io/contrib/propagators/ot v1.17.0 h1:ufo2Vsz8l76eI47jFjuVyjyB3Ae2DmfiCV/o6Vc8ii0=
go.sum:go.opentelemetry.io/contrib/propagators/ot v1.17.0/go.mod h1:SbKPj5XGp8K/sGm05XblaIABgMgw2jDczP8gGeuaVLk=
modules/caddyhttp/tracing/module.go:func (ot *Tracing) Provision(ctx caddy.Context) error {
modules/caddyhttp/tracing/module.go:        ot.logger = ctx.Logger()
modules/caddyhttp/tracing/module.go:        ot.otel, err = newOpenTelemetryWrapper(ctx, ot.SpanName)
modules/caddyhttp/tracing/module.go:func (ot *Tracing) Cleanup() error {
modules/caddyhttp/tracing/module.go:        if err := ot.otel.cleanup(ot.logger); err != nil {
modules/caddyhttp/tracing/module.go:func (ot *Tracing) ServeHTTP(w http.ResponseWriter, r *http.Request, next caddyhttp.Handler) error {
modules/caddyhttp/tracing/module.go:        return ot.otel.ServeHTTP(w, r, next)
modules/caddyhttp/tracing/module.go:func (ot *Tracing) UnmarshalCaddyfile(d *caddyfile.Dispenser) error {
modules/caddyhttp/tracing/module.go:                "span": &ot.SpanName,
modules/caddyhttp/tracing/module_test.go:                   ot := &Tracing{}
modules/caddyhttp/tracing/module_test.go:                   if err := ot.UnmarshalCaddyfile(tt.d); (err != nil) != tt.wantErr {
modules/caddyhttp/tracing/module_test.go:                   if ot.SpanName != tt.spanName {
modules/caddyhttp/tracing/module_test.go:                           t.Errorf("UnmarshalCaddyfile() SpanName = %v, want SpanName %v", ot.SpanName, tt.spanName)
modules/caddyhttp/tracing/module_test.go:                   ot := &Tracing{}
modules/caddyhttp/tracing/module_test.go:                   if err := ot.UnmarshalCaddyfile(tt.d); (err != nil) != tt.wantErr {
modules/caddyhttp/tracing/module_test.go:   ot := &Tracing{
modules/caddyhttp/tracing/module_test.go:   if err := ot.Provision(ctx); err != nil {
modules/caddyhttp/tracing/module_test.go:   if err := ot.ServeHTTP(w, req, handler); err != nil {
modules/caddyhttp/tracing/module_test.go:   ot := &Tracing{
modules/caddyhttp/tracing/module_test.go:   if err := ot.Provision(ctx); err != nil {
modules/caddyhttp/tracing/module_test.go:   if err := ot.ServeHTTP(w, req, handler); err != nil {
modules/caddyhttp/tracing/module_test.go:   ot := &Tracing{
modules/caddyhttp/tracing/module_test.go:   if err := ot.Provision(ctx); err != nil {
modules/caddyhttp/tracing/module_test.go:   if err := ot.ServeHTTP(w, req, handler); err == nil || !errors.Is(err, expectErr) {
modules/caddyhttp/tracing/tracer.go:        ot := openTelemetryWrapper{
modules/caddyhttp/tracing/tracer.go:        res, err := ot.newResource(webEngineName, version)
modules/caddyhttp/tracing/tracer.go:                return ot, fmt.Errorf("creating resource error: %w", err)
modules/caddyhttp/tracing/tracer.go:                return ot, fmt.Errorf("creating trace exporter error: %w", err)
modules/caddyhttp/tracing/tracer.go:        ot.propagators = autoprop.NewTextMapPropagator()
modules/caddyhttp/tracing/tracer.go:        ot.handler = otelhttp.NewHandler(http.HandlerFunc(ot.serveHTTP),
modules/caddyhttp/tracing/tracer.go:                ot.spanName,
modules/caddyhttp/tracing/tracer.go:                otelhttp.WithPropagators(ot.propagators),
modules/caddyhttp/tracing/tracer.go:                otelhttp.WithSpanNameFormatter(ot.spanNameFormatter),
modules/caddyhttp/tracing/tracer.go:        return ot, nil
modules/caddyhttp/tracing/tracer.go:func (ot *openTelemetryWrapper) serveHTTP(w http.ResponseWriter, r *http.Request) {
modules/caddyhttp/tracing/tracer.go:        ot.propagators.Inject(ctx, propagation.HeaderCarrier(r.Header))
modules/caddyhttp/tracing/tracer.go:func (ot *openTelemetryWrapper) ServeHTTP(w http.ResponseWriter, r *http.Request, next caddyhttp.Handler) error {
modules/caddyhttp/tracing/tracer.go:        ot.handler.ServeHTTP(w, r.WithContext(context.WithValue(r.Context(), nextCallCtxKey, n)))
modules/caddyhttp/tracing/tracer.go:func (ot *openTelemetryWrapper) cleanup(logger *zap.Logger) error {
modules/caddyhttp/tracing/tracer.go:func (ot *openTelemetryWrapper) newResource(
modules/caddyhttp/tracing/tracer.go:func (ot *openTelemetryWrapper) spanNameFormatter(operation string, r *http.Request) string {

tread is indeed a correct word but rare to encounter in code, more often a typo from thread, hence here (I guess).

Copy link
Member

Choose a reason for hiding this comment

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

Oh ... this checks spelling of variable names too?

I was kinda hoping it would just do comments and maybe strings.

I dunno. I'm still not super convinced that this is a net help for the project. I like the idea but in practice I can see it requires maintaining exceptions more than the occasional typo fix.

Copy link
Author

Choose a reason for hiding this comment

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

that's somewhat of a beauty of the codespell that it is a very basic tool which identifies words (regardless of the programming language etc) and see which ones are close hits to typos. I do once-a-while run into fixing bugs where variable in a scripted (e.g. python or JS) program was mistyped and never covered by tests, thus never detected... Of cause YMMV, but you might appreciate the low number of false positives (besides those non-descript short variable names) it ran into in caddy's entire codebase.

@yarikoptic
Copy link
Author

yarikoptic commented Mar 10, 2025

Is it possible to move that .codespellrc file into .github or something? I've been hoping to un-pollute our project root.

for the purpose of github action -- likely so (didn't check). But that would then make it trickier to run locally since default config locations codespell checks are all in root and you do not have any of those alternatives (which are mostly python world centric -- pyproject.toml, setup.cfg)

edit: is there some "standard"ish location for configs in go world?

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