refactor: simplify and de-duplicate utility helpers#26
Merged
Conversation
- Delegate array Contains to the standard library slices package - Remove the unreachable pre-Go-1.20 bytesconv build-tag file - Add typed fast paths for string conversion to avoid reflection - Collapse redundant boolean return blocks in ToBool - Extract a shared helper for file close error logging
There was a problem hiding this comment.
Pull request overview
This PR performs internal refactors and small simplifications across utility packages, aiming to reduce duplication and rely on standard library helpers where possible.
Changes:
array.Containsnow delegates toslices.Contains.bytesconvremoves the Go 1.19 fallback file and drops thego1.20build tag from the remaining implementation.convert.ToStringadds typed fast paths forstring/*string, andconvert.ToBoolis simplified with direct boolean expressions.file.Copyde-duplicates close-error handling via a sharedcloseFilehelper.
Reviewed changes
Copilot reviewed 5 out of 5 changed files in this pull request and generated 2 comments.
Show a summary per file
| File | Description |
|---|---|
| file/file.go | Extracts duplicated deferred close-error handling into closeFile. |
| convert/convert.go | Adds string fast path to ToString; simplifies ToBool numeric checks. |
| bytesconv/bytesconv.go | Removes the Go version build constraint from the remaining implementation. |
| bytesconv/bytesconv_1.19.go | Deletes the Go 1.19-specific implementation file. |
| array/array.go | Replaces manual loop with slices.Contains. |
Comments suppressed due to low confidence (1)
bytesconv/bytesconv.go:3
- Dropping the
go1.20build tag here (and removing the!go1.20implementation) effectively removes the version-gated selection described inbytesconv/README.md(which still mentions Go 1.19 support/build tags). If Go <1.20 support is no longer intended, the docs should be updated accordingly; otherwise, the 1.19 implementation/build tags need to remain.
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
- Clarify closeFile doc comment to reflect stdout printing - Add ToString test cases for string and string pointer branches - Drop stale Go 1.19 build-tag references from bytesconv README
- Redirect closeFile diagnostics to stderr to avoid corrupting piped stdout
- Cover successful copy, existing-destination error, missing-source error - Cover non-regular-file source error
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
Quality-only cleanups from a
/simplifypass over the library. No public API changes — all tests pass. One small, intentional behavior change (see below).Containsnow delegates to the stdlibslices.Containsinstead of a hand-rolled loop (same signature/behavior).bytesconv_1.19.go(//go:build !go1.20), which is unreachable under the module'sgo 1.25.10requirement. Dropped the now-always-truego1.20build tag from the survivor and renamed it tobytesconv.go.ToStringgains a typed fast path forstring/*string, avoidingfmt.Sprintfreflection on the most common input. Collapsed threeif value != 0 { return true } return falseblocks inToBooltoreturn value != 0.deferclosures inCopyinto a singlecloseFile(f, role)helper.Behavior change
fmt.Fprintf(os.Stderr, ...)) instead of stdout. Previously these messages were printed to stdout, which could corrupt a caller's intended stdout data (e.g. when piping). This is an intentional fix; the only observable difference is the stream these diagnostic lines are written to.Verification
go build ./...— cleango vet ./...— cleango test ./...— all packages passOut of scope (flagged for a follow-up
/code-review)The simplify pass also surfaced potential correctness/behavior issues that were intentionally not touched here:
ToFloat/ToBoolhave narrower numeric/pointer type coverage thanToInt(e.g.ToFloat(int64(...))returns0.0). These are behavior changes and belong in a separate review.🤖 Generated with Claude Code