refactor!: rename error vars to ErrFoo and expand lint enforcement#11
Merged
refactor!: rename error vars to ErrFoo and expand lint enforcement#11
Conversation
BREAKING CHANGE: every exported error identifier changes name to match the staticcheck ST1012 convention that the Go standard library uses. Consumers referencing these variables must update their imports: NoBaseURLError -> ErrNoBaseURL UnexpectedEmptyStringError -> ErrUnexpectedEmptyString NonEmptySuffixError -> ErrNonEmptySuffix BadParserIndexError -> ErrBadParserIndex DuplicatePartNameError -> ErrDuplicatePartName RequiredTokenError -> ErrRequiredToken InvalidIPv6HostnameError -> ErrInvalidIPv6Hostname InvalidPortError -> ErrInvalidPort EmptyPartNameError -> ErrEmptyPartName InvalidModifierError -> ErrInvalidModifier InvalidPrefixOrSuffix -> ErrInvalidPrefixOrSuffix InvalidPartNameError -> ErrInvalidPartName TypeError -> ErrType Also drops the ST1012 exclusion from .golangci.yml since the codebase now satisfies it. Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
Expand the enabled linter set in .golangci.yml (err113, goconst,
gocritic, intrange, modernize, perfsprint, thelper, unconvert, unparam,
wastedassign, whitespace) and address every new finding:
- wastedassign: collapse ExecInit's 8 dead var = "" initializations and
one in parser.go where regexpValue was assigned and immediately
reassigned.
- gocritic (assignOp / ifElseChain / typeSwitchVar): use x += y, rewrite
if-else chains to switch, switch on typed values.
- intrange: for range b.N in benchmarks, for i := range len(portValue)
in the port prefix check; keep escape.go as-is (i needed after loop)
with a //nolint rationale.
- modernize: replace interface{} with any in tests, use slices.Contains.
- perfsprint: fmt.Sprintf("%d", i) -> strconv.Itoa(i) in test names.
- unconvert: drop redundant int() around p.componentStart.
- unparam: consumeRequiredToken no longer returns its *token since
every caller discarded it.
- whitespace: trim a stray trailing newline in buildExpected and a
leading one in newExpectedResult.
- err113: hoist "invalid hostname" to errInvalidHostname at package
level, add two test-only sentinels for the WPT harness.
- goconst: extract "pattern"/"url" into initTypePattern / initTypeURL.
- thelper: t.Helper() in the three WPT test helpers.
- Drop the dead commented //p.result.Hash/Search lines in the
constructor parser and refactor the surrounding if/else into a switch.
All 365 WPT cases + the regression tests still pass; golangci-lint
reports 0 issues with the widened configuration.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
There was a problem hiding this comment.
Pull request overview
This PR performs a breaking-but-idiomatic cleanup of exported sentinel error variables (to satisfy staticcheck ST1012), and expands golangci-lint enforcement with corresponding mechanical refactors to bring the codebase into compliance.
Changes:
- Renames exported
FooErrorsentinel vars toErrFooacross the package API surface. - Expands
.golangci.ymlto enable additional linters (err113, intrange, modernize, etc.) and refactors code to satisfy them. - Modernizes tests/benchmarks and small internal refactors (e.g.,
any,slices.Contains,t.Helper(), integerrangeloops, and reduced temporary vars).
Reviewed changes
Copilot reviewed 9 out of 9 changed files in this pull request and generated no comments.
Show a summary per file
| File | Description |
|---|---|
.golangci.yml |
Enables additional linters and removes the ST1012 suppression now that error vars are renamed. |
urlpattern.go |
Renames exported errors, introduces init-type constants, and refactors init/url processing & exec flow to satisfy new lint rules. |
parser.go |
Renames exported errors, simplifies string concatenation, refactors token consumption API, and applies intrange/modernize-driven loop changes. |
parts.go |
Renames exported errors to Err* and updates call sites accordingly. |
tokenizer.go |
Renames exported TypeError to ErrType and updates wrapping site. |
urlpattern_test.go |
Updates test data types to any, uses strconv/slices, adds t.Helper() to helpers, and normalizes sentinel test errors. |
benchmark_test.go |
Updates benchmark loops to for range b.N idiom (compatible with the repo’s Go version). |
escape.go |
Adds targeted //nolint:intrange to preserve required index reuse while keeping intrange enabled. |
constructor_type_parser.go |
Applies small modernizations/switch refactors and removes dead commented assignments. |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Summary
Two commits:
refactor!: rename the 13 exportedFooErrorvars to idiomaticErrFooto satisfy staticcheck ST1012. Breaking change — consumers referencing these by name must update imports (full rename table in the commit body).chore: expand.golangci.ymlwitherr113,goconst,gocritic,intrange,modernize,perfsprint,thelper,unconvert,unparam,wastedassign,whitespace. Address every finding:ExecInit's 8 deadvar = \"\"initializations;x += y, switch-on-type, if-else → switch refactors;for range b.N/for i := range nidioms;interface{}→any,slices.Contains,strconv.Itoa;int()conversion;consumeRequiredTokendrops its unused*tokenreturn;\"invalid hostname\"to a package sentinel;\"pattern\"/\"url\"toinitTypePattern/initTypeURLconstants;t.Helper()in the three WPT helpers;//p.result.Hash = \"\"///p.result.Search = \"\"lines and collapse the surrounding if-else into a switch.Opinionated linters (
wsl,wsl_v5,lll,varnamelen,exhaustruct,gochecknoglobals,nlreturn,mnd,cyclop,gocognit,nestif,funcorder,paralleltest,noinlineerr,wrapcheck,forcetypeassert,tagliatelle,exhaustive) stay disabled — they'd require structural refactors or change error identities exposed to consumers.go test -race ./...andgolangci-lint runboth clean against the widened configuration.