Add lint/CI configs, update release, refactor env API, enhance as #10
Conversation
|
This pull request sets up GitHub code scanning for this repository. Once the scans have completed and the checks have passed, the analysis results for this pull request branch will appear on this overview. Once you merge this pull request, the 'Security' tab will show more code scanning analysis results (for example, for the default branch). Depending on your configuration and choice of analysis tool, future pull requests will be annotated with code scanning analysis results. For more information about GitHub code scanning, check out the documentation. |
WalkthroughRefactors env parsing/loading and testing seams, adds new as converters and error types, replaces several public APIs (Override→Parse/ParseInto, Vars type→Vars func, State.Reset→Restore), introduces internal IO abstractions and reader-based Load/LoadFile/LoadFromReader, and updates CI/linting/docs and many tests. Changes
Sequence Diagram(s)sequenceDiagram
autonumber
actor App
participant Env as env.Parse / env.ParseInto
participant Internal as internal.LookupEnv
participant Cnv as Converter
participant Err as ParseError
App->>Env: Parse(name, cnv, [def])
Env->>Internal: LookupEnv(name)
alt not set and default provided
Env-->>App: return default, nil
else not set and no default
Env-->>App: error ErrNotSet wrapped in ParseError
else set
Env->>Cnv: cnv(value)
alt convert ok
Env-->>App: value, nil
else convert err
Env-->>App: ParseError(wrapped InvalidValueError)
end
end
sequenceDiagram
autonumber
actor App
participant Env as env.Load
participant Int as internal (FileExists/Open/Stat)
participant Reader as LoadFromReader
participant State as env.State()
App->>Env: Load(filenames...)
Env->>Int: FileExists(".env")?
alt .env exists
Env->>Env: prepend ".env"
end
Env->>Env: sanitize, dedupe, filter missing
Env->>State: state := State()
loop per file
Env->>Int: Open(filename)
alt open ok
Env->>Reader: LoadFromReader(reader)
alt parse ok
Reader-->>Env: nil
else parse error
Env->>State: state.Restore()
Env-->>App: FileError / ErrInvalidEntry
end
else open error
Env-->>App: FileError(filename, err)
end
end
Env-->>App: nil
Estimated code review effort🎯 4 (Complex) | ⏱️ ~60 minutes Possibly related PRs
Poem
Pre-merge checks and finishing touches❌ Failed checks (1 warning)
✅ Passed checks (2 passed)
✨ Finishing touches
🧪 Generate unit tests
📜 Recent review detailsConfiguration used: Path: .coderabbit.yaml Review profile: ASSERTIVE Plan: Pro ⛔ Files ignored due to path filters (1)
📒 Files selected for processing (37)
💤 Files with no reviewable changes (4)
🧰 Additional context used🧠 Learnings (16)📚 Learning: 2025-09-21T03:03:26.651ZApplied to files:
📚 Learning: 2025-09-24T23:04:07.744ZApplied to files:
📚 Learning: 2024-07-04T08:27:07.225ZApplied to files:
📚 Learning: 2025-09-25T22:32:23.679ZApplied to files:
📚 Learning: 2025-09-24T22:24:17.657ZApplied to files:
📚 Learning: 2024-07-04T09:04:46.594ZApplied to files:
📚 Learning: 2025-09-15T03:59:10.905ZApplied to files:
📚 Learning: 2025-09-21T03:06:50.819ZApplied to files:
📚 Learning: 2025-09-12T18:30:13.036ZApplied to files:
📚 Learning: 2025-09-24T22:33:15.369ZApplied to files:
📚 Learning: 2025-09-13T00:47:52.975ZApplied to files:
📚 Learning: 2025-09-24T23:20:43.209ZApplied to files:
📚 Learning: 2024-07-04T08:06:24.732ZApplied to files:
📚 Learning: 2025-09-24T23:02:26.831ZApplied to files:
📚 Learning: 2025-09-21T02:35:04.766ZApplied to files:
📚 Learning: 2025-09-25T03:13:16.923ZApplied to files:
🧬 Code graph analysis (25)state.go (1)
as/duration_test.go (2)
parse.go (2)
load_test.go (5)
as/url.go (1)
state_test.go (2)
as/portNo.go (1)
as/internal/funcs.go (1)
env.go (1)
vars_test.go (2)
as/duration.go (1)
vars.go (1)
as/int_test.go (2)
as/int.go (1)
internal/funcs_test.go (1)
as/string_test.go (1)
errors_test.go (1)
load.go (4)
as/bool_test.go (1)
env_test.go (3)
as/url_test.go (2)
parse_test.go (3)
errors.go (1)
as/portNo_test.go (2)
as/filename_test.go (2)
⏰ Context from checks skipped due to timeout of 90000ms. You can increase the timeout in your CodeRabbit configuration to a maximum of 15 minutes (900000ms). (1)
🔇 Additional comments (32)
Tip 👮 Agentic pre-merge checks are now available in preview!Pro plan users can now enable pre-merge checks in their settings to enforce checklists before merging PRs.
Please see the documentation for more information. Example: reviews:
pre_merge_checks:
custom_checks:
- name: "Undocumented Breaking Changes"
mode: "warning"
instructions: |
Pass/fail criteria: All breaking changes to public APIs, CLI flags, environment variables, configuration keys, database schemas, or HTTP/GraphQL endpoints must be documented in the "Breaking Change" section of the PR description and in CHANGELOG.md. Exclude purely internal or private changes (e.g., code not exported from package entry points or explicitly marked as internal).Please share your feedback with us on this Discord post. Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
coderabbitai
bot
changed the title
![coderabbitai[bot]](https://avatars.githubusercontent.com/in/347564?s=60&v=4){kind=small}
There was a problem hiding this comment.
Actionable comments posted: 37
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (8)
state.go (2)
16-17: Docs still reference Reset(); update to Restore()Public docs/examples under State() still say Reset, which no longer exists. This is user-facing; fix to avoid confusion.
Apply:
-// the test has run (by deferring a call to the Reset method on the -// returned state value). +// the test has run (by deferring a call to the Restore method on the +// returned state value). @@ -// defer env.State().Reset() +// defer env.State().Restore()Also applies to: 29-29
3-6: Use internal.Setenv alias for testability; keep behavior identicalUsing os.Setenv here bypasses the internal alias that’s provided specifically for monkey-patching in tests. Switch to internal.Setenv.
Apply:
import ( "os" "strings" + "github.com/blugnu/env/internal" ) @@ - _ = os.Setenv(k, v) + _ = internal.Setenv(k, v)Also applies to: 65-66
.golangci.yml (1)
31-53: Misplaced linter settings block; use top-level linters-settingsgolangci-lint expects linter configuration under the top-level key linters-settings, not linters.settings. As-is, these settings will be ignored.
Apply:
- settings: - depguard: - rules: - tests: - files: - - $test - list-mode: lax - allow: ["*"] - - # disabled until I figure out a way to FAIL on a PR branch and only WARN on feature branches - # godox: - # keywords: - # - TODO - - revive: - rules: - - name: package-comments - disabled: true - - staticcheck: - checks: - - -QF1008 # "could remove embedded field <name> from selector"; sometimes it is useful (if unnecessary) to disambiguate +linters-settings: + depguard: + rules: + tests: + files: + - $test + list-mode: lax + allow: ["*"] + + # disabled until I figure out a way to FAIL on a PR branch and only WARN on feature branches + # godox: + # keywords: + # - TODO + + revive: + rules: + - name: package-comments + disabled: true + + staticcheck: + checks: + - -QF1008 # "could remove embedded field <name> from selector"; sometimes it is useful (if unnecessary) to disambiguateparse.go (1)
52-54: Docs contradict implementation for ErrNotSet.Code wraps ErrNotSet in ParseError; comment claims nil error. Fix the doc.
-// // any error resulting from parsing the environment variable; -// // if the error is ErrNotSet a nil error is returned +// // any error resulting from parsing the environment variable; +// // if the variable is not set and no default is provided, a [ParseError] +// // wrapping [ErrNotSet] is returnedREADME.md (2)
16-16: Go version badge alt text is inaccurate (uses generics and errors.Join).The code uses generics (Go ≥1.18) and errors.Join (Go ≥1.20). Update the badge alt text accordingly.
- <img alt="go version >= 1.14" src="https://img.shields.io/github/go-mod/go-version/blugnu/env?style=flat-square"/> + <img alt="go version >= 1.20" src="https://img.shields.io/github/go-mod/go-version/blugnu/env?style=flat-square"/>
39-45: Document minimum supported Go version.Make the requirement explicit near Installation to prevent confusion.
## Installation ```bash go get github.com/blugnu/env
+> Requires Go 1.20+.
</blockquote></details> <details> <summary>load.go (2)</summary><blockquote> `30-31`: **Parameter name mismatch in docs.** Signature uses “filenames”; update the parameter name in the docs for consistency. ```diff -// files: ...string // 0..n file path(s) +// filenames: ...string // 0..n file path(s)
37-41: Over-specific error formatting in docs.Docs prescribe “path/to/file: error”, but errors are wrapped in env.FileError and formatting depends on FileError.Error(). Rephrase to avoid promising a specific string shape.
-// with the file path that caused the error: -// -// "path/to/file: error" +// and annotated with the filename via env.FileError in the error chain.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (41)
.coderabbit-golangci.yml(1 hunks).coderabbit.yaml(1 hunks).github/dependabot.yml(1 hunks).github/workflows/release.yml(1 hunks).golangci.yml(1 hunks)README.md(3 hunks)as/as_test.go(0 hunks)as/bool.go(1 hunks)as/bool_test.go(1 hunks)as/duration.go(1 hunks)as/duration_test.go(1 hunks)as/errors.go(1 hunks)as/filename.go(1 hunks)as/filename_test.go(1 hunks)as/funcs.go(0 hunks)as/int.go(1 hunks)as/int_test.go(1 hunks)as/internal/funcs.go(1 hunks)as/portNo.go(2 hunks)as/portNo_test.go(1 hunks)as/string_test.go(1 hunks)as/url.go(2 hunks)as/url_test.go(1 hunks)env.go(4 hunks)env_test.go(1 hunks)errors.go(1 hunks)errors_test.go(1 hunks)funcs.go(0 hunks)funcs_test.go(0 hunks)go.mod(1 hunks)internal/consts.go(1 hunks)internal/func_test.go(1 hunks)internal/funcs.go(1 hunks)load.go(2 hunks)load_test.go(1 hunks)parse.go(4 hunks)parse_test.go(1 hunks)state.go(3 hunks)state_test.go(1 hunks)vars.go(1 hunks)vars_test.go(1 hunks)
💤 Files with no reviewable changes (4)
- funcs_test.go
- as/funcs.go
- funcs.go
- as/as_test.go
🧰 Additional context used
🧠 Learnings (1)
📚 Learning: 2024-07-04T08:06:24.732Z
Learnt from: deltics
PR: blugnu/env#2
File: load_test.go:3-10
Timestamp: 2024-07-04T08:06:24.732Z
Learning: The `io/fs` package is used in the `load_test.go` file, specifically `fs.ErrNotExist`.
Applied to files:
internal/func_test.goload_test.go
🧬 Code graph analysis (24)
as/filename_test.go (2)
as/filename.go (1)
Filename(10-24)as/errors.go (1)
ErrFileDoesNotExist(6-6)
as/url.go (1)
as/internal/funcs.go (1)
ParseUrl(7-7)
as/string_test.go (1)
as/string.go (1)
String(14-16)
as/filename.go (1)
as/errors.go (1)
ErrFileDoesNotExist(6-6)
as/bool_test.go (1)
as/bool.go (1)
Bool(25-37)
as/int_test.go (1)
as/int.go (1)
Int(19-25)
as/portNo_test.go (2)
as/portNo.go (1)
PortNo(31-45)errors.go (1)
RangeError(181-184)
internal/func_test.go (1)
internal/funcs.go (3)
FileExists(27-30)Open(14-14)NewFileReader(43-45)
as/duration_test.go (2)
as/duration.go (2)
Duration(21-27)DurationIn(49-66)as/errors.go (1)
ErrNotADuration(8-8)
as/url_test.go (2)
as/url.go (1)
AbsoluteURL(31-40)as/errors.go (1)
ErrNotAnAbsoluteURL(10-10)
load_test.go (5)
internal/funcs.go (5)
FileExists(27-30)NewFileReader(43-45)Environ(11-11)LookupEnv(17-17)Setenv(20-20)state.go (1)
State(38-40)load.go (3)
Load(69-101)LoadFile(113-129)LoadFromReader(147-187)internal/consts.go (1)
EnvFile(4-4)errors.go (2)
FileError(25-28)ErrInvalidEntry(11-11)
as/portNo.go (1)
errors.go (1)
RangeError(181-184)
vars.go (1)
internal/funcs.go (2)
Environ(11-11)LookupEnv(17-17)
parse_test.go (3)
internal/funcs.go (1)
LookupEnv(17-17)parse.go (2)
Parse(61-79)ParseInto(101-109)errors.go (3)
ErrNotSet(19-19)InvalidValueError(125-128)ParseError(70-73)
env.go (1)
internal/funcs.go (2)
LookupEnv(17-17)Unsetenv(23-23)
as/int.go (1)
as/errors.go (1)
ErrNotAnInteger(7-7)
as/duration.go (1)
as/errors.go (1)
ErrNotADuration(8-8)
load.go (4)
internal/consts.go (1)
EnvFile(4-4)internal/funcs.go (4)
FileExists(27-30)NewFileReader(43-45)LookupEnv(17-17)Setenv(20-20)errors.go (3)
NewFileError(30-35)ErrInvalidEntry(11-11)ErrSetFailed(15-15)state.go (1)
State(38-40)
vars_test.go (2)
state.go (1)
State(38-40)vars.go (1)
Vars(28-59)
state.go (1)
internal/funcs.go (1)
Setenv(20-20)
errors_test.go (1)
errors.go (5)
FileError(25-28)NewFileError(30-35)ParseError(70-73)InvalidValueError(125-128)RangeError(181-184)
parse.go (2)
errors.go (3)
ParseError(70-73)InvalidValueError(125-128)ErrNotSet(19-19)internal/funcs.go (1)
LookupEnv(17-17)
env_test.go (3)
state.go (1)
State(38-40)internal/funcs.go (3)
Setenv(20-20)LookupEnv(17-17)Unsetenv(23-23)env.go (6)
Clear(11-13)Get(28-30)IsSet(35-38)Lookup(56-58)Set(74-76)Unset(91-98)
state_test.go (2)
internal/funcs.go (2)
Environ(11-11)Setenv(20-20)state.go (1)
State(38-40)
🪛 GitHub Actions: release
go.mod
[error] 1-1: Run # a script to extract build information from ./go.mod failed: file does not exist (./go.mod)
🔇 Additional comments (27)
go.mod (1)
3-5: Go 1.23 bump and test-lib upgrade — ensure CI/toolchain alignment and sums updated.
- Confirm the reusable workflow sets up Go 1.23; otherwise builds will fail.
- Ensure
go.sumwas regenerated forgithub.com/blugnu/test v0.12.0.Run:
#!/bin/bash set -euo pipefail # Confirm files present test -f go.mod && echo "go.mod: OK" || { echo "go.mod missing"; exit 1; } if [[ -f go.sum ]]; then echo "go.sum: OK"; else echo "go.sum missing"; fi # Show go directive and test dep awk '/^go [0-9]+\.[0-9]+/ {print "go directive:", $0}' go.mod grep -nE 'github.com/blugnu/test' go.mod || true.github/workflows/release.yml (1)
6-7: ```shell
#!/bin/bash
set -euo pipefailecho "PWD: $(pwd)"
echo
echo "---- Search for 'blugnu/.reusable' ----"
if command -v rg >/dev/null 2>&1; then
rg -n --hidden "blugnu/.reusable" || true
else
grep -RIn --hidden "blugnu/.reusable" || true
fiecho
echo "---- Matches for 'uses:.*blugnu/.reusable' ----"
if command -v rg >/dev/null 2>&1; then
rg -n --hidden "uses:.*blugnu/.reusable" -S || true
else
grep -RIn --hidden "uses:.*blugnu/.reusable" || true
fiecho
echo "---- Show .github/workflows/release.yml (lines 1-120) ----"
if [ -f .github/workflows/release.yml ]; then
nl -ba -w3 -s' ' .github/workflows/release.yml | sed -n '1,120p'
else
echo "file not found: .github/workflows/release.yml"
fi</blockquote></details> <details> <summary>.github/dependabot.yml (1)</summary><blockquote> `1-11`: **LGTM.** Sensible defaults (weekly cadence, PR cap, reviewer/label set). </blockquote></details> <details> <summary>as/string_test.go (1)</summary><blockquote> `11-23`: **LGTM.** Straightforward positive-case test; consistent with the test harness used elsewhere. </blockquote></details> <details> <summary>state.go (1)</summary><blockquote> `61-67`: **Confirm intent to drop Setenv errors** Errors from Setenv are intentionally discarded via blank identifier. If this is to appease errcheck only, consider at least a debug log in tests, or document the rationale. </blockquote></details> <details> <summary>as/bool_test.go (1)</summary><blockquote> `28-51`: **LGTM: comprehensive truthy/falsey coverage and error assertions** Cases align with as.Bool behavior (extended yes/no forms, wrapped strconv.ErrSyntax). Table harness usage is clean. </blockquote></details> <details> <summary>.golangci.yml (1)</summary><blockquote> `50-53`: **Verify that staticcheck code QF1008 is valid in your toolchain** Ensure your golangci-lint/staticcheck version recognizes QF1008; otherwise this line will be ignored. Run locally: ```shell #!/bin/bash # Show supported staticcheck checks (requires golangci-lint installed) golangci-lint linters | sed -n '/staticcheck/,/^[[:alnum:]]/p'as/filename_test.go (1)
28-51: LGTM: clear coverage of exists/missing/invalid filename pathsAssertions correctly use sentinel (ErrFileDoesNotExist) and syscall.EINVAL for invalid names; tidy table-driven style.
as/url.go (1)
10-13: ```shell
#!/bin/bash
set -euo pipefailecho "Repo root: $(pwd)"
echo "Listing as/ directory (if present):"
ls -la as || trueecho
echo "Show as/url.go (if exists):"
if [ -f as/url.go ]; then
nl -ba as/url.go | sed -n '1,240p'
else
echo "as/url.go not found; searching repository for AbsoluteURL, ErrNotAnAbsoluteURL, IsAbs, ParseUrl, ParseURL..."
rg -n --hidden -S "func\s+AbsoluteURL" || true
rg -n --hidden -S "ErrNotAnAbsoluteURL|not an absolute URI" || true
rg -n --hidden -S "IsAbs(" || true
rg -n --hidden -S "ParseUrl|ParseURL" || true
fi</blockquote></details> <details> <summary>internal/func_test.go (1)</summary><blockquote> `37-50`: **LGTM: negative path correctly verifies error passthrough from Open** The failure stub and assertions are straightforward and robust. </blockquote></details> <details> <summary>as/portNo.go (1)</summary><blockquote> `32-41`: **LGTM: correct bounds and consistent error wrapping.** Uses math.MaxUint16 and wraps errors with context. Behavior matches tests. </blockquote></details> <details> <summary>as/url_test.go (1)</summary><blockquote> `25-47`: **LGTM: covers absolute, relative, and invalid cases.** Good use of type assertion for *url.Error and ErrNotAnAbsoluteURL via errors.Is semantics. </blockquote></details> <details> <summary>as/duration_test.go (2)</summary><blockquote> `16-36`: **LGTM: Duration happy-path and error-path are covered.** Assertions align with as.Duration behavior. --- `47-81`: **LGTM: DurationIn covers unit application, precedence of explicit duration, and syntax errors.** Clear separation of scenarios; matches implementation comments. </blockquote></details> <details> <summary>vars_test.go (1)</summary><blockquote> `26-46`: **LGTM: verifies both “all vars” and “selected vars” flows.** Good isolation via State().Restore() and Clearenv; assertions match new Vars API. </blockquote></details> <details> <summary>env_test.go (2)</summary><blockquote> `15-27`: **Tests: structure and isolation LGTM.** Good use of State().Restore(), t.Setenv, and assertions across Clear/Get/IsSet/Lookup/Set. Also applies to: 29-48, 50-63, 65-86, 88-101 --- `137-149`: ```shell #!/bin/bash set -euo pipefail echo "===== Searching for 'func Unset(' =====" rg -nP --hidden --follow --no-ignore 'func\s+Unset\(' || true echo echo "===== Searching for 'Unsetenv' occurrences/definitions =====" rg -nP --hidden --follow --no-ignore '\bUnsetenv\b' || true echo if [ -f go.mod ]; then echo "===== go.mod go version =====" rg -n '^go\s' go.mod || true echo "===== go.mod (head) =====" sed -n '1,80p' go.mod else echo "===== no go.mod found in repo root =====" fi echo echo "===== Showing files with 'func Unset(' (first 240 lines) =====" for f in $(rg -lP --hidden --follow --no-ignore 'func\s+Unset\(' || true); do echo echo "---- $f ----" sed -n '1,240p' "$f" done echo echo "===== Showing files that reference or define 'Unsetenv' (first 240 lines) =====" for f in $(rg -lP --hidden --follow --no-ignore '\bUnsetenv\b' || true); do echo echo "---- $f ----" sed -n '1,240p' "$f" done echo echo "===== Searching for call sites of 'Unset(' =====" rg -nP --hidden --follow --no-ignore '\bUnset\(' || true echo echo "===== Showing env_test.go files (lines 1-300) =====" find . -type f -name 'env_test.go' -print -exec sed -n '1,300p' {} \; || trueerrors.go (1)
17-20: Message change for ErrNotSet — verify external callers.The message changed to “variable not set.” If any consumers compare error strings (ill-advised but common), this is breaking. Confirm dependents use errors.Is(err, env.ErrNotSet) instead.
state_test.go (1)
12-23: LGTM: State snapshot reflects os.Environ().parse_test.go (7)
14-28: LGTM: Happy path parse via LookupEnv stub.
30-44: LGTM: Not-set path asserts ErrNotSet and zero value.
46-61: LGTM: Conversion error wraps with ParseError→InvalidValueError.
63-77: LGTM: Default used when not set.
79-93: LGTM: Default ignored when value set.
111-126: LGTM: ParseInto updates target on success.
128-145: Avoid future flakiness: don’t parallelize tests that stub globals.Because tests replace internal.LookupEnv (a global), adding t.Parallel would race. Keep these serial or gate the stub with a mutex if parallelizing later.
load_test.go (1)
16-107: Good DI of file I/O; cases read well.Stubbing internal.FileExists/NewFileReader and restoring state per test is clean and reliable.
There was a problem hiding this comment.
Actionable comments posted: 4
♻️ Duplicate comments (1)
README.md (1)
32-41: Resolved: unchecked boxes replaced.This addresses the prior feedback about unchecked task boxes.
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
📒 Files selected for processing (1)
README.md(3 hunks)
🔇 Additional comments (5)
README.md (5)
5-5: Tagline reads well.Concise and on-message.
76-86: Required value example looks correct.API usage and error handling are clear.
91-101: ParseInto example looks correct.Good illustration of pointer destination and converter usage.
118-126: Vars example is clear.Nicely communicates “only if set” behavior.
103-116: Docs: rename cfg.Url → cfg.URL; ParseInto already accepts an optional default
- ParseInto signature is ParseInto[T any](target *T, name string, cnv ConversionFunc[T], def ...T) — optional default supported.
- Update README example (README.md, around lines 109–111).
- errs = append(errs, env.ParseInto(&cfg.Url, "AUTH_SERVICE_URL", as.AbsoluteURL)) + errs = append(errs, env.ParseInto(&cfg.URL, "AUTH_SERVICE_URL", as.AbsoluteURL))Likely an incorrect or invalid review comment.
deltics
force-pushed
the
develop
branch
2 times, most recently
from
7571c76 to
dca2cf3
Compare
There was a problem hiding this comment.
Actionable comments posted: 14
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (3)
.golangci.yml (1)
31-53: Misplaced linter settings; should be top-level linters-settings, not linters.settings.As written, depguard/revive/staticcheck configs won’t apply.
- settings: - depguard: - rules: - tests: - files: - - $test - list-mode: lax - allow: ["*"] - - # disabled until I figure out a way to FAIL on a PR branch and only WARN on feature branches - # godox: - # keywords: - # - TODO - - revive: - rules: - - name: package-comments - disabled: true - - staticcheck: - checks: - - -QF1008 +linters-settings: + depguard: + rules: + tests: + files: + - $test + list-mode: lax + allow: ["*"] + + # disabled until I figure out a way to FAIL on a PR branch and only WARN on feature branches + # godox: + # keywords: + # - TODO + + revive: + rules: + - name: package-comments + disabled: true + + staticcheck: + checks: + - -QF1008state.go (1)
13-30: Docs still reference Reset; rename to Restore in comments and example.Keep API docs consistent with the method name.
Apply:
-// the test has run (by deferring a call to the Reset method on the +// the test has run (by deferring a call to the Restore method on the // returned state value). @@ -// defer env.State().Reset() +// defer env.State().Restore()env.go (1)
56-58: Use internal.LookupEnv in Lookup as well.Keeps lookup behavior mockable and consistent with IsSet/Unset.
func Lookup(name string) (string, bool) { - return os.LookupEnv(name) + return internal.LookupEnv(name) }
♻️ Duplicate comments (39)
.coderabbit-golangci.yml (1)
1-1: Remove stale v1 stub; consolidate on .golangci.yml.This file isn’t a valid golangci-lint config and is no longer needed now that CodeRabbit supports v2. Keeping it is confusing.
.coderabbit.yaml (1)
1-6: Point CodeRabbit’s golangci-lint at the project config for consistency.Autodetection should find .golangci.yml, but making it explicit avoids drift.
reviews: profile: assertive request_changes_workflow: true tools: + golangci-lint: + config_file: .golangci.yml yamllint: enabled: falseas/internal/funcs.go (1)
5-8: Rename ParseUrl → ParseURL (initialism).Go style prefers URL over Url. Update call sites accordingly.
var ( - // alias for [url.Parse] - ParseUrl = url.Parse + // ParseURL is an alias for [url.Parse]. + ParseURL = url.Parse )as/bool.go (1)
9-14: Fix GoDoc grammar and tighten phrasing.“an Boolean” → “a Boolean”; keep comments idiomatic and concise.
-// Bool converts a string to an Boolean. Conversion is initially attempted -// -// using [strconv.ParseBool]. If that fails, the value is checked for -// -// "y"/"yes" or "n"/"no" (case-insensitive). -// -// -// If the value is not supported by [strconv.ParseBool] and is not one of -// -// the additional values above, an error is returned. +// Bool converts a string to a Boolean. +// It first tries strconv.ParseBool. If that fails, it accepts "y"/"yes" and "n"/"no" (case-insensitive). +// Returns the parsed value or an error if unsupported.as/errors.go (1)
10-10: Fix wording to match identifier: URL vs URI.Align the public error message with the identifier and related docs/usages.
- ErrNotAnAbsoluteURL = errors.New("not an absolute URI") + ErrNotAnAbsoluteURL = errors.New("not an absolute URL")errors.go (1)
37-46: Use consistent "env.FileError" prefix and include filename.Match formatting used by other error types; improves diagnostics.
// Error implements the error interface. func (err FileError) Error() string { - switch { - case err.err != nil: - return fmt.Sprintf("file error: %v", err.err) - - default: - return "file error" - } + switch { + case err.err != nil && err.Filename != "": + return fmt.Sprintf("env.FileError: %s: %v", err.Filename, err.err) + case err.err != nil: + return fmt.Sprintf("env.FileError: %v", err.err) + case err.Filename != "": + return fmt.Sprintf("env.FileError: %s", err.Filename) + default: + return "env.FileError" + } }as/int.go (2)
3-6: Import errors to correctly join sentinel and cause.import ( - "fmt" + "errors" + "fmt" "strconv" )
22-22: Fix invalid error wrapping: only one %w allowed.Preserve both ErrNotAnInteger and the original error via errors.Join.
- return 0, fmt.Errorf("as.Int: %w: %w", ErrNotAnInteger, err) + return 0, fmt.Errorf("as.Int: %w", errors.Join(ErrNotAnInteger, err))internal/consts.go (1)
1-6: Prefer filepath.Join for OS-agnostic paths.-package internal - -const ( - EnvFile = ".env" - EnvFilePath = "./" + EnvFile -) +package internal + +import "path/filepath" + +const ( + EnvFile = ".env" + EnvFilePath = filepath.Join(".", EnvFile) +)as/url.go (1)
32-35: Use initialism: ParseURL (not ParseUrl). Also align docs to “URL”.Apply in this file and rename the alias in as/internal accordingly.
- u, err := internal.ParseUrl(s) + u, err := internal.ParseURL(s)Additional doc tweak (outside diff hunk, for consistency):
-// AbsoluteURL converts a string to an absolute URI. An absolute URI is a URI -// that includes a scheme, host, and path. The function returns an error if the -// string is not an absolute URI. +// AbsoluteURL converts a string to an absolute URL. An absolute URL includes a +// scheme (per url.URL.IsAbs). The function returns an error if the string is +// not an absolute URL.internal/func_test.go (1)
23-35: Return a real, closeable file from the Open stub and close it to avoid leaks.Returning &os.File{} constructs an invalid handle and the test never closes the reader.
Apply:
Run(Test("when Open is successful", func() { // arrange - defer Restore(Original(&internal.Open).ReplacedBy(func(string) (*os.File, error) { - return &os.File{}, nil - })) + defer Restore(Original(&internal.Open).ReplacedBy(func(string) (*os.File, error) { + f, err := os.CreateTemp(t.TempDir(), "open-ok-*") + if err != nil { + t.Fatalf("setup temp file: %v", err) + } + return f, nil + })) @@ // assert Expect(err).IsNil() Expect(result).IsNotNil() + _ = result.Close() }))as/filename.go (1)
15-23: Validate empty path and reject directories (Filename should only accept files).Guard empty string and ensure the path is not a directory.
Apply:
- if _, err := os.Stat(v); err != nil { + if v == "" { + return handleError(ErrFileDoesNotExist) + } + fi, err := os.Stat(v) + if err != nil { if errors.Is(err, os.ErrNotExist) { return handleError(ErrFileDoesNotExist) } return handleError(err) - } + } + if fi.IsDir() { + return handleError(ErrFileDoesNotExist) + } return v, nilas/portNo_test.go (3)
3-11: Strengthen tests: assert RangeError bounds.Also import errors and math for the checks.
Apply:
import ( + "errors" + "math" "strconv" "testing" @@ "github.com/blugnu/env" "github.com/blugnu/env/as" )
22-25: Add Min/Max assertions for -1 case.Apply:
Case("-1", func(result int, err error) { Expect(err).Is(env.RangeError[int]{}) + var re env.RangeError[int] + if errors.As(err, &re) { + Expect(re.Min).To(Equal(0)) + Expect(re.Max).To(Equal(int(math.MaxUint16))) + } Expect(result).To(Equal(0)) }),
42-45: Add Min/Max assertions for 65536 case.Apply:
Case("65536", func(result int, err error) { Expect(err).Is(env.RangeError[int]{}) + var re env.RangeError[int] + if errors.As(err, &re) { + Expect(re.Min).To(Equal(0)) + Expect(re.Max).To(Equal(int(math.MaxUint16))) + } Expect(result).To(Equal(0)) }),as/int_test.go (1)
13-20: Fix double %w in as.Int (as/int.go:22).
fmt.Errorf currently uses two %w verbs; replace with a single %w wrapping errors.Join so errors.Is and wrapping behave correctly.-import "fmt" +import ( + "errors" + "fmt" +) func Int(s string) (int, error) { i, err := strconv.Atoi(s) if err != nil { - return 0, fmt.Errorf("as.Int: %w: %w", ErrNotAnInteger, err) + return 0, fmt.Errorf("as.Int: %w", errors.Join(ErrNotAnInteger, err)) } return i, nil }env.go (2)
32-37: Route IsSet through internal.LookupEnv for consistency/testability.Use the internal seam like other helpers.
Apply this diff:
func IsSet(name string) bool { - _, ok := os.LookupEnv(name) + _, ok := internal.LookupEnv(name) return ok }
92-95: Prefix error with env.Unset (not os.Unsetenv).Aligns with exported API naming used elsewhere.
- if err := internal.Unsetenv(k); err != nil { - return fmt.Errorf("os.Unsetenv: %s: %w", k, err) + if err := internal.Unsetenv(k); err != nil { + return fmt.Errorf("env.Unset: %s: %w", k, err) }vars.go (5)
3-8: Import cleanup and internal seam for Environ.Use internal.Environ and drop the direct os import.
-import ( - "os" - "strings" - - "github.com/blugnu/env/internal" -) +import ( + "strings" + + "github.com/blugnu/env/internal" +)
35-41: Source env via internal.Environ().Keeps all OS hooks behind internal.
- case 0: // all environment variables - src = os.Environ() + case 0: // all environment variables + src = internal.Environ()
42-47: Trim names before lookup to avoid misses with whitespace.Return the trimmed key from the handler.
default: // only the named variables (if set) src = names fn = func(s string) (string, string, bool) { - v, isSet := internal.LookupEnv(s) - return s, v, isSet + k := strings.TrimSpace(s) + v, isSet := internal.LookupEnv(k) + return k, v, isSet }
50-56: Avoid double-trimming; rely on handlers to return cleaned keys.Removes redundant TrimSpace in the loop.
result := make(map[string]string, len(src)) for _, s := range src { if k, v, isSet := fn(s); isSet { - k = strings.TrimSpace(k) result[k] = v } }
10-28: Document trimming semantics in the return description.Prevents surprises for callers.
-// map[string]string // a map of environment variables where the key is the -// // name of the environment variable +// map[string]string // a map of environment variables where the key is the +// // name of the environment variable; keys are trimmed of +// // surrounding whitespaceas/duration.go (2)
3-7: Fix invalid multi-%w wrapping; also trim input before parsing.
fmt.Errorfsupports only one %w. Current code won’t compile. Also, accept surrounding whitespace. Join errors once and wrap.import ( + "errors" "fmt" "strconv" + "strings" "time" ) // Duration parses a string into a time.Duration. The string must contain // a valid duration expression (e.g., "1h30m"). func Duration(s string) (time.Duration, error) { - dur, err := time.ParseDuration(s) + s = strings.TrimSpace(s) + dur, err := time.ParseDuration(s) if err != nil { - return 0, fmt.Errorf("as.Duration: %w: %w", ErrNotADuration, err) + return 0, fmt.Errorf("as.Duration: %w", errors.Join(ErrNotADuration, err)) } return dur, nil }Also applies to: 21-27
49-65: Preserve both root causes on integer-parse fallback; trim input.Currently the fallback discards the duration-parse error and doesn’t trim. Join both errors and keep context.
func DurationIn(u time.Duration) func(string) (time.Duration, error) { return func(s string) (time.Duration, error) { - i, err := strconv.Atoi(s) - if err != nil { - // attempting to parse as a duration expression is a fallback, - // but if that fails then the issue is that the value is not - // an integer, so any duration parsing error is discarded in - // favor of the original integer parsing error - if dur, err := time.ParseDuration(s); err == nil { - return dur, nil - } - - return 0, fmt.Errorf("as.DurationIn(%s): %w", u, err) - } - - return time.Duration(i) * u, nil + s = strings.TrimSpace(s) + + if i, err := strconv.Atoi(s); err == nil { + return time.Duration(i) * u, nil + } else { + if dur, durErr := time.ParseDuration(s); durErr == nil { + return dur, nil + } else { + return 0, fmt.Errorf("as.DurationIn(%s): %w", u, errors.Join(err, durErr)) + } + } } }parse.go (1)
101-109: Guard against nil target in ParseInto to avoid panic.Dereferencing a nil target panics.
-import "github.com/blugnu/env/internal" +import ( + "fmt" + "github.com/blugnu/env/internal" +) @@ func ParseInto[T any](target *T, name string, cnv ConversionFunc[T], def ...T) error { + if target == nil { + return fmt.Errorf("env.ParseInto(%q): target is nil", name) + } v, err := Parse(name, cnv, def...) if err != nil { return err } *target = v return nil }state_test.go (1)
25-27: Add a safety-net restore to prevent leaking a cleared env on panic.Capture original env and defer restore before clearing.
func TestState_Restore(t *testing.T) { With(t) + orig := env.State() + defer orig.Restore() @@ // assert Expect(os.Getenv("VAR1")).To(Equal("value1")) Expect(os.Getenv("VAR2")).To(Equal("value2"))Also applies to: 36-41
internal/funcs.go (3)
9-24: Mark globals as intentional test seams (silence gochecknoglobals).These are purposeful seams for substitution in tests.
-var ( +//nolint:gochecknoglobals // intentional test seams for OS primitives +var (
26-31: Clarify and correct FileExists semantics.Current impl returns true on permission or other errors. If “exists” should mean “Stat succeeded,” use err == nil and update the comment.
-// FileExists returns true if a file with the given filename exists. +// FileExists reports whether the path exists (returns true only if os.Stat succeeds). var FileExists = func(filename string) bool { _, err := os.Stat(filename) - return !errors.Is(err, os.ErrNotExist) + return err == nil }
43-45: Mark NewFileReader seam and intent.Add nolint to avoid churn.
-var NewFileReader = func(path string) (io.ReadCloser, error) { +//nolint:gochecknoglobals // test seam +var NewFileReader = func(path string) (io.ReadCloser, error) { return Open(path) }errors_test.go (2)
100-107: Remove duplicated test case.Same scenario listed twice.
- {scenario: "FileError/different Filename/matching error", - target: env.NewFileError("other.txt", sut.Unwrap()), - expected: false, - },
55-58: Add coverage for FileError.Unwrap for symmetry.Mirror ParseError/InvalidValueError Unwrap tests.
func TestFileError_Is(t *testing.T) { With(t) sut := env.NewFileError("file.txt", errors.New("some error")) + + // also verify Unwrap symmetry + t.Run("Unwrap returns inner error", func(t *testing.T) { + With(t) + err := errors.New("inner") + got := env.NewFileError("any.txt", err).Unwrap() + Expect(got).To(Equal(err)) + })parse_test.go (1)
17-21: Deduplicate LookupEnv stubbing with a tiny helper.Reduces repetition and centralizes restore handling.
package env_test @@ import ( "errors" "strconv" "testing" . "github.com/blugnu/test" "github.com/blugnu/env" "github.com/blugnu/env/internal" ) +// withLookupEnv replaces internal.LookupEnv for the test and returns a restore func. +func withLookupEnv(val string, ok bool) func() { + return Restore(Original(&internal.LookupEnv).ReplacedBy( + func(string) (string, bool) { return val, ok }, + )) +} @@ - defer Restore(Original(&internal.LookupEnv).ReplacedBy(func(string) (string, bool) { - return "123", true - })) + defer withLookupEnv("123", true)() @@ - defer Restore(Original(&internal.LookupEnv).ReplacedBy(func(string) (string, bool) { - return "", false - })) + defer withLookupEnv("", false)() @@ - defer Restore(Original(&internal.LookupEnv).ReplacedBy(func(string) (string, bool) { - return "not-a-number", true - })) + defer withLookupEnv("not-a-number", true)() @@ - defer Restore(Original(&internal.LookupEnv).ReplacedBy(func(string) (string, bool) { - return "", false - })) + defer withLookupEnv("", false)() @@ - defer Restore(Original(&internal.LookupEnv).ReplacedBy(func(string) (string, bool) { - return "123", true - })) + defer withLookupEnv("123", true)() @@ - defer Restore(Original(&internal.LookupEnv).ReplacedBy(func(string) (string, bool) { - return "abc", true - })) + defer withLookupEnv("abc", true)() @@ - defer Restore(Original(&internal.LookupEnv).ReplacedBy(func(string) (string, bool) { - return "123", true - })) + defer withLookupEnv("123", true)() @@ - defer Restore(Original(&internal.LookupEnv).ReplacedBy(func(string) (string, bool) { - return "not a number", true - })) + defer withLookupEnv("not a number", true)()Also applies to: 33-37, 51-54, 67-70, 83-86, 99-102, 116-119, 133-136
load_test.go (2)
198-206: Add test: comment lines are ignored.Covers “# …” with/without leading spaces.
@@ Case("contains empty lines", testcase{ content: "\nVAR1=value1\n\nVAR2=value2\n", assert: func(err error) { Expect(err).IsNil() Expect(os.Environ()).To(ContainItems([]string{ "VAR1=value1", "VAR2=value2", })) }, }), + + Case("comment lines (with and without leading spaces) are ignored", testcase{ + content: "# c1\n # c2\nVAR=ok\n", + assert: func(err error) { + Expect(err).IsNil() + Expect(os.Getenv("VAR")).To(Equal("ok")) + }, + }),
220-227: Add test: scanner/read error restores env and returns error.Simulate reader failure to cover scanner.Err path once implementation is fixed.
@@ Case("setting variable fails", testcase{ content: "VAR1=value1\nVAR2=value2\n", setErr: os.ErrInvalid, assert: func(err error) { Expect(err).Is(os.ErrInvalid) Expect(os.Environ()).Should(BeEmpty()) }, }), + Case("scanner/read error rolls back changes", testcase{ + content: "", // unused + assert: func(err error) { + Expect(err).Is(os.ErrInvalid) + Expect(os.Environ()).Should(BeEmpty()) + }, + }),And in the arrange section, inject a failing reader for that case:
@@ - src := io.NopCloser(bytes.NewReader([]byte(tc.content))) + var src io.ReadCloser + if tc.content == "" && t.Name() == "scanner/read error rolls back changes" { + type failingReader struct{ n int } + func (r *failingReader) Read(p []byte) (int, error) { + if r.n > 0 { return 0, os.ErrInvalid } + r.n++ + copy(p, []byte("VAR=ok\nTOO-LONG")) + return len("VAR=ok\nTOO-LONG"), nil + } + src = io.NopCloser(&failingReader{}) + } else { + src = io.NopCloser(bytes.NewReader([]byte(tc.content))) + }load.go (4)
44-55: Docs contradict behavior for missing .env and explicit filenames. Align with implementation and tests.Missing files are ignored; no error is returned when nothing is loadable.
-// The function will ALWAYS attempt to load variables from a ".env" file, as follows: +// The function will always attempt to load variables from a ".env" file, as follows: @@ -// If ".env" (or "./.env") is included in the files parameter it will be loaded -// in the order specified relative to other files; if the ".env" file does not exist -// an error will be included in the returned error. +// If ".env" (or "./.env") is included in the files parameter it will be loaded +// in the order specified relative to other files; if the ".env" file does not exist +// it is ignored without error (consistent with other filenames). @@ -// If ".env" is NOT explicitly specified it will be loaded before any other files; if it does -// not exist it is ignored without error unless no other files are specified. +// If ".env" is NOT explicitly specified it will be loaded before any other files; if it does +// not exist it is ignored without error. @@ -// If no files are specified the function will attempt to load variables from ".env" -// and will return an error if the file does not exist. +// If no files are specified the function will attempt to load variables from ".env". +// If it does not exist, nothing is loaded and the function returns nil.
150-153: Fix error context prefix for LoadFromReader.Make source clear in logs.
- return fmt.Errorf("env/internal.Load: %w", err) + return fmt.Errorf("env.LoadFromReader: %w", err)
182-186: Honor scanner errors to preserve transactional guarantee.Restore env and return on scan/read failure.
- if err := errors.Join(errs...); err != nil { + // capture scanning/reader errors (e.g., token too long, reader error) + if scanErr := scanner.Err(); scanErr != nil { + return handleError(scanErr) + } + if err := errors.Join(errs...); err != nil { return handleError(err) }
146-147: Typo: “occurrs” → “occurs”.-// error // any error that occurrs while loading or applying variables +// error // any error that occurs while loading or applying variables
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (39)
.coderabbit-golangci.yml(1 hunks).coderabbit.yaml(1 hunks).golangci.yml(1 hunks)README.md(4 hunks)as/as_test.go(0 hunks)as/bool.go(1 hunks)as/bool_test.go(1 hunks)as/duration.go(1 hunks)as/duration_test.go(1 hunks)as/errors.go(1 hunks)as/filename.go(1 hunks)as/filename_test.go(1 hunks)as/funcs.go(0 hunks)as/int.go(1 hunks)as/int_test.go(1 hunks)as/internal/funcs.go(1 hunks)as/portNo.go(2 hunks)as/portNo_test.go(1 hunks)as/string_test.go(1 hunks)as/url.go(2 hunks)as/url_test.go(1 hunks)env.go(4 hunks)env_test.go(1 hunks)errors.go(1 hunks)errors_test.go(1 hunks)funcs.go(0 hunks)funcs_test.go(0 hunks)go.mod(1 hunks)internal/consts.go(1 hunks)internal/func_test.go(1 hunks)internal/funcs.go(1 hunks)load.go(2 hunks)load_test.go(1 hunks)parse.go(4 hunks)parse_test.go(1 hunks)state.go(3 hunks)state_test.go(1 hunks)vars.go(1 hunks)vars_test.go(1 hunks)
💤 Files with no reviewable changes (4)
- as/as_test.go
- as/funcs.go
- funcs_test.go
- funcs.go
🧰 Additional context used
🧠 Learnings (4)
📚 Learning: 2025-09-12T18:30:13.024Z
Learnt from: deltics
PR: blugnu/env#10
File: .coderabbit.yaml:4-8
Timestamp: 2025-09-12T18:30:13.024Z
Learning: CodeRabbit now supports golangci-lint v2, so separate v1 configuration files for CodeRabbit are no longer necessary. Both v1 and v2 versions are supported by CodeRabbit's tooling.
Applied to files:
.coderabbit-golangci.yml.coderabbit.yaml.golangci.yml
📚 Learning: 2024-07-04T08:27:07.225Z
Learnt from: deltics
PR: blugnu/env#2
File: load.go:63-91
Timestamp: 2024-07-04T08:27:07.225Z
Learning: When suggesting improvements for error handling, ensure to verify that all types of errors are adequately covered in the tests, including multiple errors being joined.
Applied to files:
as/portNo_test.goerrors_test.go
📚 Learning: 2024-07-04T08:06:24.732Z
Learnt from: deltics
PR: blugnu/env#2
File: load_test.go:3-10
Timestamp: 2024-07-04T08:06:24.732Z
Learning: The `io/fs` package is used in the `load_test.go` file, specifically `fs.ErrNotExist`.
Applied to files:
internal/func_test.go
📚 Learning: 2024-07-04T09:04:46.594Z
Learnt from: deltics
PR: blugnu/env#2
File: load.go:94-121
Timestamp: 2024-07-04T09:04:46.594Z
Learning: When suggesting the use of `bufio.Scanner` for reading lines in files, consider the potential impact on testing complexity, especially if efficiency is not critical.
Applied to files:
load_test.goload.go
🧬 Code graph analysis (25)
as/filename_test.go (2)
as/filename.go (1)
Filename(10-24)as/errors.go (1)
ErrFileDoesNotExist(6-6)
as/portNo_test.go (2)
as/portNo.go (1)
PortNo(31-45)errors.go (1)
RangeError(181-184)
vars_test.go (3)
state.go (1)
State(38-40)internal/funcs.go (1)
Setenv(20-20)vars.go (1)
Vars(28-59)
as/url.go (1)
as/internal/funcs.go (1)
ParseUrl(7-7)
internal/func_test.go (1)
internal/funcs.go (3)
FileExists(27-30)Open(14-14)NewFileReader(43-45)
as/string_test.go (1)
as/string.go (1)
String(14-16)
as/int_test.go (1)
as/int.go (1)
Int(19-25)
parse_test.go (3)
internal/funcs.go (1)
LookupEnv(17-17)parse.go (2)
Parse(61-79)ParseInto(101-109)errors.go (3)
ErrNotSet(19-19)InvalidValueError(125-128)ParseError(70-73)
as/int.go (1)
as/errors.go (1)
ErrNotAnInteger(7-7)
as/duration_test.go (2)
as/duration.go (2)
Duration(21-27)DurationIn(49-66)as/errors.go (1)
ErrNotADuration(8-8)
as/portNo.go (1)
errors.go (1)
RangeError(181-184)
state.go (1)
internal/funcs.go (1)
Setenv(20-20)
vars.go (1)
internal/funcs.go (2)
Environ(11-11)LookupEnv(17-17)
parse.go (2)
errors.go (3)
ParseError(70-73)InvalidValueError(125-128)ErrNotSet(19-19)internal/funcs.go (1)
LookupEnv(17-17)
env.go (1)
internal/funcs.go (2)
LookupEnv(17-17)Unsetenv(23-23)
errors_test.go (1)
errors.go (5)
FileError(25-28)NewFileError(30-35)ParseError(70-73)InvalidValueError(125-128)RangeError(181-184)
as/url_test.go (2)
as/url.go (1)
AbsoluteURL(31-40)as/errors.go (1)
ErrNotAnAbsoluteURL(10-10)
load_test.go (5)
internal/funcs.go (5)
FileExists(27-30)NewFileReader(43-45)Environ(11-11)LookupEnv(17-17)Setenv(20-20)state.go (1)
State(38-40)load.go (3)
Load(69-101)LoadFile(113-129)LoadFromReader(147-187)internal/consts.go (1)
EnvFile(4-4)errors.go (2)
FileError(25-28)ErrInvalidEntry(11-11)
state_test.go (2)
internal/funcs.go (2)
Environ(11-11)Setenv(20-20)state.go (1)
State(38-40)
env_test.go (3)
state.go (1)
State(38-40)internal/funcs.go (3)
Setenv(20-20)LookupEnv(17-17)Unsetenv(23-23)env.go (6)
Clear(11-13)Get(28-30)IsSet(35-38)Lookup(56-58)Set(74-76)Unset(91-98)
as/filename.go (1)
as/errors.go (1)
ErrFileDoesNotExist(6-6)
as/bool_test.go (1)
as/bool.go (1)
Bool(15-28)
as/duration.go (1)
as/errors.go (1)
ErrNotADuration(8-8)
load.go (4)
internal/consts.go (1)
EnvFile(4-4)internal/funcs.go (4)
FileExists(27-30)NewFileReader(43-45)LookupEnv(17-17)Setenv(20-20)errors.go (3)
NewFileError(30-35)ErrInvalidEntry(11-11)ErrSetFailed(15-15)state.go (1)
State(38-40)
errors.go (1)
as/filename.go (1)
Filename(10-24)
🪛 GitHub Actions: release
go.mod
[error] 1-1: Run script to extract build information from ./go.mod failed: go.mod file not found (./go.mod).
🔇 Additional comments (12)
go.mod (1)
3-5: Align CI with Go 1.23 and fix release workflow path to go.mod.
- Confirmed: go.mod is at the repository root; .github/workflows/release.yml delegates to blugnu/.reusable/.github/workflows/[email protected] (no local working-directory or setup-go in release.yml).
- Action: Update .github/workflows/release.yml to either pass the module path to the reusable workflow (check that workflow's input name — e.g. with: path: . / module-path: .) or set working-directory to the repo root before invoking it; also ensure the workflow uses Go 1.23 (pass go-version: "1.23" or have setup-go read go.mod).
as/string_test.go (1)
11-23: LGTM.Test reads cleanly and matches as.String behavior.
.golangci.yml (1)
2-19: Invalid key: linters.default; use enable-all in v2.golangci-lint v2 expects linters.enable-all (or explicit enables). “default: all” won’t be honored.
-version: "2" -linters: - default: all +version: "2" +linters: + enable-all: true disable: - err113 - embeddedstructfieldcheck - exhaustruct - funlen - gochecknoglobals - godot - inamedparam - interfacebloat - lll - nlreturn - noinlineerr - paralleltest - varnamelen - wsl - wsl_v5⛔ Skipped due to learnings
Learnt from: deltics PR: blugnu/env#10 File: .coderabbit.yaml:4-8 Timestamp: 2025-09-12T18:30:13.024Z Learning: CodeRabbit now supports golangci-lint v2, so separate v1 configuration files for CodeRabbit are no longer necessary. Both v1 and v2 versions are supported by CodeRabbit's tooling.as/filename_test.go (1)
25-33: LGTM on happy-path assertion.Checks for existing file and equality are clear and sufficient.
as/portNo.go (1)
32-44: LGTM: clear range check and contextual wrapping.Using math.MaxUint16 and env.RangeError improves clarity; error wrapping preserves strconv.ErrSyntax for errors.Is in tests.
as/url_test.go (3)
25-31: LGTM on absolute URL success case.Checks scheme/host directly; concise and robust.
33-39: LGTM on relative-URL error assertion.Assuming env.InvalidValueError unwraps ErrNotAnAbsoluteURL, this is correct.
If InvalidValueError stops unwrapping in future, adjust the assertion to errors.Is(err, as.ErrNotAnAbsoluteURL).
41-47: LGTM on invalid-URL type assertion.Type-checking *url.Error keeps the parse-error contract stable.
as/duration_test.go (1)
13-37: Good coverage and assertions for Duration.The cases align with the new API and error semantics.
env_test.go (1)
104-151: Clarify Unset semantics on partial failures; add a targeted case.Decide whether Unset should stop on first error (current) and allow partial effects, or attempt all. Add a case to lock this in.
Proposed test addition:
@@ Case("error when unsetting", func() { @@ Expect(os.Getenv("VAR2")).To(Equal("value2")) }), + + Case("partial progress when later unset fails", func() { + // arrange: first unset succeeds (actually unsets), second fails + unsetErr := errors.New("unset error after first") + call := 0 + defer Restore(Original(&internal.Unsetenv).ReplacedBy(func(k string) error { + call++ + if call == 1 { + return os.Unsetenv(k) // emulate a real successful unset + } + return unsetErr + })) + + // act + err := env.Unset("VAR1", "VAR2") + + // assert + Expect(err).Is(unsetErr) + Expect(os.Getenv("VAR1")).To(Equal("")) + Expect(os.Getenv("VAR2")).To(Equal("value2")) + }),vars_test.go (1)
12-47: Vars tests read well and match the new API.README.md (1)
16-17: No action required — README badge matches go.mod.
go.mod declares 1.23; README badge alt text is "go version >= 1.23".
There was a problem hiding this comment.
Actionable comments posted: 20
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (9)
as/url.go (2)
10-13: Doc/code mismatch: IsAbs only checks scheme; doc says scheme, host, and path.Either enforce host (and path if truly required) or relax the doc. Suggest requiring a non-empty scheme and host.
-// AbsoluteURL converts a string to an absolute URI. An absolute URI is a URI -// that includes a scheme, host, and path. The function returns an error if the -// string is not an absolute URI. +// AbsoluteURL converts a string to an absolute URL. +// It requires a non-empty scheme and host; otherwise returns an error.
32-39: Enforce host presence (not just scheme).
url.URL.IsAbs()doesn’t require Host. Add a host check to match the contract.- u, err := internal.ParseURL(s) + u, err := internal.ParseURL(s) if err != nil { return nil, err //nolint:wrapcheck // returns the parse error from [url.Parse] } - if !u.IsAbs() { + if !u.IsAbs() || u.Host == "" { return nil, env.InvalidValueError{Value: s, Err: ErrNotAnAbsoluteURL} }state.go (2)
13-18: Docstring still references Reset; should be Restore.Update the narrative to consistently use Restore().
-// the test has run (by deferring a call to the Reset method on the +// the test has run (by deferring a call to the Restore method on the
27-30: Example uses Reset; should use Restore for consistency.Align the example with the new API.
-// defer env.State().Reset() +// defer env.State().Restore()as/portNo.go (1)
3-9: Trim whitespace before parsing.Helps with inputs like " 443 " and aligns with other as.* helpers that TrimSpace.
import ( "fmt" "math" + "strings" "strconv" "github.com/blugnu/env" )- i, err := strconv.Atoi(s) + s = strings.TrimSpace(s) + i, err := strconv.Atoi(s)env.go (1)
56-58: Use internal.LookupEnv in Lookup for seam consistency.Matches IsSet and improves testability.
-func Lookup(name string) (string, bool) { - return os.LookupEnv(name) -} +func Lookup(name string) (string, bool) { + return internal.LookupEnv(name) +}errors.go (3)
115-129: Support pointer targets in ParseError.Is for consistency with FileError.Improves ergonomics with errors.Is(&env.ParseError{...}, err).
-func (e ParseError) Is(target error) bool { - if target, ok := target.(ParseError); ok { - return (target.VariableName == "" || e.VariableName == target.VariableName) && - (target.Err == nil || errors.Is(e.Err, target.Err)) - } - return false -} +func (e ParseError) Is(target error) bool { + switch t := target.(type) { + case ParseError: + return (t.VariableName == "" || e.VariableName == t.VariableName) && + (t.Err == nil || errors.Is(e.Err, t.Err)) + case *ParseError: + if t == nil { + return false + } + return (t.VariableName == "" || e.VariableName == t.VariableName) && + (t.Err == nil || errors.Is(e.Err, t.Err)) + default: + return false + } +}
170-183: Ditto for InvalidValueError.Is: accept pointer targets.-func (e InvalidValueError) Is(target error) bool { - if target, ok := target.(InvalidValueError); ok { - return (target.Value == "" || e.Value == target.Value) && - (target.Err == nil || errors.Is(e.Err, target.Err)) - } - return false -} +func (e InvalidValueError) Is(target error) bool { + switch t := target.(type) { + case InvalidValueError: + return (t.Value == "" || e.Value == t.Value) && + (t.Err == nil || errors.Is(e.Err, t.Err)) + case *InvalidValueError: + if t == nil { + return false + } + return (t.Value == "" || e.Value == t.Value) && + (t.Err == nil || errors.Is(e.Err, t.Err)) + default: + return false + } +}
214-225: And for RangeError[T].Is: accept pointer targets.-func (e RangeError[T]) Is(target error) bool { - if target, ok := target.(RangeError[T]); ok { - return e == target || (target == RangeError[T]{}) - } - return false -} +func (e RangeError[T]) Is(target error) bool { + switch t := target.(type) { + case RangeError[T]: + return e == t || (t == RangeError[T]{}) + case *RangeError[T]: + if t == nil { + return false + } + return e == *t || (*t == RangeError[T]{}) + default: + return false + } +}
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (37)
.golangci.yml(1 hunks)README.md(4 hunks)as/as_test.go(0 hunks)as/bool.go(1 hunks)as/bool_test.go(1 hunks)as/duration.go(1 hunks)as/duration_test.go(1 hunks)as/errors.go(1 hunks)as/filename.go(1 hunks)as/filename_test.go(1 hunks)as/funcs.go(0 hunks)as/int.go(1 hunks)as/int_test.go(1 hunks)as/internal/funcs.go(1 hunks)as/portNo.go(2 hunks)as/portNo_test.go(1 hunks)as/string_test.go(1 hunks)as/url.go(2 hunks)as/url_test.go(1 hunks)env.go(4 hunks)env_test.go(1 hunks)errors.go(1 hunks)errors_test.go(1 hunks)funcs.go(0 hunks)funcs_test.go(0 hunks)go.mod(1 hunks)internal/consts.go(1 hunks)internal/func_test.go(1 hunks)internal/funcs.go(1 hunks)load.go(1 hunks)load_test.go(1 hunks)parse.go(2 hunks)parse_test.go(1 hunks)state.go(3 hunks)state_test.go(1 hunks)vars.go(1 hunks)vars_test.go(1 hunks)
💤 Files with no reviewable changes (4)
- funcs_test.go
- as/funcs.go
- funcs.go
- as/as_test.go
🧰 Additional context used
🧠 Learnings (9)
📚 Learning: 2025-09-15T03:59:10.905Z
Learnt from: deltics
PR: blugnu/env#10
File: internal/consts.go:3-6
Timestamp: 2025-09-15T03:59:10.905Z
Learning: In Go, filepath.Join cannot be used in const declarations since it's a function call. For simple relative path prefixes like "./", string concatenation is cross-platform compatible and preferable to using vars with filepath.Join.
Applied to files:
internal/consts.go
📚 Learning: 2024-07-04T08:06:24.732Z
Learnt from: deltics
PR: blugnu/env#2
File: load_test.go:3-10
Timestamp: 2024-07-04T08:06:24.732Z
Learning: The `io/fs` package is used in the `load_test.go` file, specifically `fs.ErrNotExist`.
Applied to files:
internal/func_test.gointernal/funcs.goas/filename_test.go
📚 Learning: 2025-09-12T18:30:13.036Z
Learnt from: deltics
PR: blugnu/env#10
File: .coderabbit.yaml:4-8
Timestamp: 2025-09-12T18:30:13.036Z
Learning: CodeRabbit now supports golangci-lint v2, so separate v1 configuration files for CodeRabbit are no longer necessary. Both v1 and v2 versions are supported by CodeRabbit's tooling.
Applied to files:
.golangci.yml
📚 Learning: 2025-09-13T00:47:52.975Z
Learnt from: deltics
PR: blugnu/env#10
File: as/int.go:3-6
Timestamp: 2025-09-13T00:47:52.975Z
Learning: fmt.Errorf supports multiple %w verbs in a single format string. When multiple %w verbs are present, the returned error implements an Unwrap method returning a []error containing all the %w operands in the order they appear in the arguments. This allows wrapping multiple errors in a single fmt.Errorf call.
Applied to files:
as/int_test.goas/int.go
📚 Learning: 2024-07-04T09:04:46.594Z
Learnt from: deltics
PR: blugnu/env#2
File: load.go:94-121
Timestamp: 2024-07-04T09:04:46.594Z
Learning: When suggesting the use of `bufio.Scanner` for reading lines in files, consider the potential impact on testing complexity, especially if efficiency is not critical.
Applied to files:
load.goload_test.go
📚 Learning: 2025-09-21T02:35:04.766Z
Learnt from: deltics
PR: blugnu/env#10
File: load.go:163-171
Timestamp: 2025-09-21T02:35:04.766Z
Learning: The default bufio.Scanner uses ScanLines which automatically removes both \r and \n characters when reading lines, so manual CRLF handling is not needed when using scanner.Scan().
Applied to files:
load.go
📚 Learning: 2024-07-04T08:27:07.225Z
Learnt from: deltics
PR: blugnu/env#2
File: load.go:63-91
Timestamp: 2024-07-04T08:27:07.225Z
Learning: When suggesting improvements for error handling, ensure to verify that all types of errors are adequately covered in the tests, including multiple errors being joined.
Applied to files:
errors_test.goas/portNo_test.go
📚 Learning: 2025-09-21T03:06:50.819Z
Learnt from: deltics
PR: blugnu/env#10
File: state_test.go:25-42
Timestamp: 2025-09-21T03:06:50.819Z
Learning: When testing environment state management functions, don't use those same functions as safety mechanisms in tests. This creates circular dependencies where test infrastructure relies on the system under test, potentially causing cascading failures.
Applied to files:
state_test.go
📚 Learning: 2025-09-21T03:03:26.651Z
Learnt from: deltics
PR: blugnu/env#10
File: state.go:65-66
Timestamp: 2025-09-21T03:03:26.651Z
Learning: In the blugnu/env codebase, test seams (like internal package wrappers around OS functions) should only be introduced where required for testing, not for consistency alone. The practice is to use seams only where needed rather than universally wrapping all OS calls.
Applied to files:
internal/funcs.gostate.go
🧬 Code graph analysis (25)
as/string_test.go (1)
as/string.go (1)
String(14-16)
as/url.go (1)
as/internal/funcs.go (1)
ParseURL(7-7)
internal/func_test.go (1)
internal/funcs.go (3)
FileExists(35-38)Open(16-16)NewFileReader(51-53)
as/internal/funcs.go (1)
parse.go (1)
Parse(54-72)
as/duration_test.go (2)
as/duration.go (2)
Duration(14-21)DurationIn(32-50)as/errors.go (2)
ErrNotADuration(22-22)ErrNotAnInteger(21-21)
as/bool_test.go (1)
as/bool.go (1)
Bool(15-28)
env.go (1)
internal/funcs.go (3)
LookupEnv(19-19)Setenv(25-25)Unsetenv(28-28)
as/int_test.go (2)
as/int.go (1)
Int(19-25)as/errors.go (1)
ErrNotAnInteger(21-21)
load.go (4)
state.go (1)
State(38-40)internal/consts.go (1)
EnvFile(6-6)internal/funcs.go (5)
FileExists(35-38)NewFileReader(51-53)ScanLines(22-22)LookupEnv(19-19)Setenv(25-25)errors.go (3)
NewFileError(33-38)ErrInvalidEntry(11-11)ErrSetFailed(19-19)
as/duration.go (1)
as/errors.go (2)
ErrNotADuration(22-22)ErrNotAnIntegerOrDuration(25-25)
load_test.go (5)
internal/funcs.go (6)
FileExists(35-38)NewFileReader(51-53)Setenv(25-25)Environ(13-13)LookupEnv(19-19)ScanLines(22-22)state.go (1)
State(38-40)load.go (3)
Load(32-71)LoadFile(83-99)LoadFromReader(109-152)internal/consts.go (1)
EnvFile(6-6)errors.go (2)
FileError(28-31)ErrInvalidEntry(11-11)
errors_test.go (1)
errors.go (5)
FileError(28-31)NewFileError(33-38)ParseError(84-87)InvalidValueError(139-142)RangeError(195-198)
state_test.go (2)
internal/funcs.go (2)
Environ(13-13)Setenv(25-25)state.go (1)
State(38-40)
as/portNo_test.go (2)
as/portNo.go (1)
PortNo(31-45)errors.go (1)
RangeError(195-198)
vars.go (1)
internal/funcs.go (1)
LookupEnv(19-19)
parse.go (2)
errors.go (4)
ParseError(84-87)InvalidValueError(139-142)ErrNotSet(15-15)ErrTargetIsNil(22-22)internal/funcs.go (1)
LookupEnv(19-19)
as/url_test.go (2)
as/url.go (1)
AbsoluteURL(31-40)as/errors.go (1)
ErrNotAnAbsoluteURL(23-23)
parse_test.go (3)
internal/funcs.go (1)
LookupEnv(19-19)parse.go (2)
Parse(54-72)ParseInto(94-106)errors.go (4)
ErrNotSet(15-15)ParseError(84-87)InvalidValueError(139-142)ErrTargetIsNil(22-22)
vars_test.go (2)
state.go (1)
State(38-40)vars.go (1)
Vars(28-59)
as/filename_test.go (2)
as/filename.go (1)
Filename(10-32)as/errors.go (2)
ErrFileDoesNotExist(19-19)ErrFilenameIsDirectory(20-20)
as/filename.go (1)
as/errors.go (2)
ErrFileDoesNotExist(19-19)ErrFilenameIsDirectory(20-20)
as/int.go (1)
as/errors.go (1)
ErrNotAnInteger(21-21)
env_test.go (3)
state.go (1)
State(38-40)internal/funcs.go (3)
Setenv(25-25)LookupEnv(19-19)Unsetenv(28-28)env.go (6)
Clear(11-13)Get(28-30)IsSet(35-38)Lookup(56-58)Set(74-76)Unset(91-98)
as/portNo.go (1)
errors.go (1)
RangeError(195-198)
state.go (1)
internal/funcs.go (1)
Setenv(25-25)
🪛 GitHub Actions: release
go.mod
[error] 1-1: Script step 'a script to extract build information from ./go.mod' failed: file does not exist (./go.mod). Command: if [[ ! -f "$modfile" ]]; then echo "::error::file does not exist ($modfile)".
🔇 Additional comments (34)
as/int.go (1)
22-22: Good: preserves sentinel and cause with multi-%w.fmt.Errorf with multiple %w is correct here and supported; errors.Is/As will work with ErrNotAnInteger and the original cause.
Please ensure tests assert both chains:
- errors.Is(err, ErrNotAnInteger)
- errors.Is(err, strconv.ErrSyntax) (for a representative bad value)
as/filename.go (1)
19-29: Confirm desired symlink semantics.os.Stat follows symlinks. If you intend to reject symlinks-to-directories or treat broken symlinks distinctly, consider os.Lstat and explicit ModeSymlink checks.
vars_test.go (1)
37-45: Add a case asserting trimming of specified names.Covers the trimming behavior and prevents regressions.
Case("specified variables (including ones not set)", func() { // act result := env.Vars("VAR1", "VAR3") @@ })) }), + + Case("trims whitespace in specified names", func() { + // act + result := env.Vars(" VAR1\t", "\tVAR3 ") + + // assert + Expect(result).To(EqualMap(map[string]string{ + "VAR1": "value1", + })) + }),vars.go (3)
10-28: Document trimming semantics in the API comment.Clarify that keys are trimmed; aligns docs with behavior.
// # parameters // -// names ...string // (optional) names of environment variables to return; +// names ...string // (optional) names of environment variables to return; // // if no names are provided the returned map contains all -// // current environment variables. +// // current environment variables. Keys are trimmed of surrounding whitespace.
34-41: Use internal.Environ for consistency/testability.Keep env sourcing behind the internal seam (mirrors LookupEnv).
switch len(names) { case 0: // all environment variables - src = os.Environ() + src = internal.Environ() fn = func(s string) (string, string, bool) { k, v, _ := strings.Cut(s, "=") return k, v, true }Also remove the now-unneeded os import.
-import ( - "os" - "strings" - - "github.com/blugnu/env/internal" -) +import ( + "strings" + + "github.com/blugnu/env/internal" +)
42-55: Trim before lookup and avoid double‑trimming.Trim the key once in the case handler; drop the loop‑level TrimSpace.
default: // only the named variables (if set) src = names fn = func(s string) (string, string, bool) { - v, isSet := internal.LookupEnv(s) - return s, v, isSet + k := strings.TrimSpace(s) + v, isSet := internal.LookupEnv(k) + return k, v, isSet } } result := make(map[string]string, len(src)) for _, s := range src { if k, v, isSet := fn(s); isSet { - k = strings.TrimSpace(k) result[k] = v } }#!/bin/bash # Sanity: find call sites relying on whitespace in names (should be none) rg -n --type=go 'Vars\([^)]*\)' -C2go.mod (1)
5-5: Confirm dependency and rungo mod tidywith Go 1.23.Tests across the repo reference the github.com/blugnu/test DSL (e.g. vars_test.go, parse_test.go, errors_test.go, many as/*_test.go), so keeping
require github.com/blugnu/test v0.12.0is correct — rungo mod tidyusing the Go 1.23 toolchain to updatego.sumand ensure CI reproducibility.as/string_test.go (1)
11-23: LGTM for basic happy-path coverage.Test is clear and matches the trivial implementation.
as/int_test.go (1)
13-43: LGTM; good coverage of bounds and sentinel.Assertions for Min/Max int32, ErrSyntax, and ErrNotAnInteger look solid.
internal/funcs.go (2)
51-53: Mark NewFileReader as an intentional global seam.Add nolint to silence gochecknoglobals for this test seam.
-var NewFileReader = func(path string) (io.ReadCloser, error) { +//nolint:gochecknoglobals // test seam +var NewFileReader = func(path string) (io.ReadCloser, error) { return Open(path) }
31-38: Fix panic and clarify FileExists contract.
fimay be nil whenos.Statreturns a non-ENOENT error, causingfi.IsDir()to panic. Decide on the contract and implement safely. Recommend: return true only on a successful stat of a non-directory.-// FileExists returns true if a file with the given filename exists. -// Note that this function returns false for directories and returns -// -// true for files, even if the file is not accessible, e.g. due to -// permission errors. +// FileExists reports whether the path exists and is a regular file. +// It returns true only when os.Stat succeeds and the path is not a directory. var FileExists = func(filename string) bool { - fi, err := os.Stat(filename) - return !fi.IsDir() && !errors.Is(err, os.ErrNotExist) + fi, err := os.Stat(filename) + if err != nil { + return false + } + return !fi.IsDir() }as/bool.go (1)
9-14: Fix GoDoc grammar and make it idiomatic.Use “a Boolean” and condense to standard GoDoc without headings.
-// Bool converts a string to an Boolean. Conversion is initially attempted -// -// using [strconv.ParseBool]. If that fails, the value is checked for -// "y"/"yes" or "n"/"no" (case-insensitive). -// -// If the value is not supported by [strconv.ParseBool] and is not one of -// the additional values above, an error is returned. +// Bool converts a string to a Boolean. +// It first tries strconv.ParseBool (case-insensitive). +// Additionally accepts "y"/"yes" and "n"/"no" (case-insensitive). +// Returns the parsed value or an error for unsupported input.as/portNo_test.go (3)
23-29: Also assert RangeError bounds using constants.Strengthen the “-1” case.
- rangeErr := env.RangeError[int]{} - Expect(errors.As(err, &rangeErr)).To(BeTrue()) - Expect(rangeErr.Min).To(Equal(0)) - Expect(rangeErr.Max).To(Equal(65535)) + rangeErr := env.RangeError[int]{} + Expect(errors.As(err, &rangeErr)).To(BeTrue()) + Expect(rangeErr.Min).To(Equal(0)) + Expect(rangeErr.Max).To(Equal(int(math.MaxUint16))) Expect(result).To(Equal(0))
3-12: Import math to assert the upper bound symbolically.Use
math.MaxUint16to lock the contract.import ( "errors" + "math" "strconv" "testing"
46-49: Strengthen the overflow case: assert RangeError bounds.Mirror the “-1” checks here too.
- Case("65536", func(result int, err error) { - Expect(err).Is(env.RangeError[int]{}) - Expect(result).To(Equal(0)) - }), + Case("65536", func(result int, err error) { + Expect(err).Is(env.RangeError[int]{}) + var rangeErr env.RangeError[int] + if errors.As(err, &rangeErr) { + Expect(rangeErr.Min).To(Equal(0)) + Expect(rangeErr.Max).To(Equal(int(math.MaxUint16))) + } + Expect(result).To(Equal(0)) + }),as/portNo.go (1)
36-42: Behavior and error wrapping LGTM; confirm tests use errors.Is/As.Wrapping strconv and RangeError with %w is correct; ensure tests/assertions rely on errors.Is/As, not string matching.
as/url_test.go (1)
25-49: Solid coverage and assertions for absolute vs relative/invalid.Tests reflect the wrapped error behavior and parse errors correctly.
as/filename_test.go (1)
28-69: LGTM; platform-robust invalid-path assertion.Good move to avoid OS-specific syscall expectations.
env.go (1)
32-38: IsSet implementation via internal.LookupEnv looks good.Consistent with internal seams used elsewhere.
state_test.go (1)
25-42: Restore test exercises the public API correctly.Assertions validate end state; setup via t.Setenv keeps it clean.
state.go (1)
61-66: Clarify intentional ignoring of Setenv errors during restore.If best‑effort restore is intended, add a brief comment so linters/readers know this is deliberate; otherwise, consider collecting the first error and exposing it (API change).
for _, e := range s { k, v, _ := strings.Cut(e, "=") - _ = os.Setenv(k, v) + // best-effort: ignore Setenv errors during restore + _ = os.Setenv(k, v) }Note: Respecting project preference to avoid unnecessary seams; not suggesting routing via internal.Setenv here per maintainer guidance.
as/duration_test.go (1)
56-73: Add missing edge cases: "1.5h" and whitespace handling.Round out coverage per earlier suggestion.
@@ Case("ignores unit for valid duration expression", testcase{ input: "1h", unit: time.Second, assert: func(result time.Duration, err error) { Expect(err).IsNil() Expect(result).To(Equal(time.Hour)) }, }), + Case("decimal duration expression is valid", testcase{ + input: "1.5h", + unit: time.Second, + assert: func(result time.Duration, err error) { + Expect(err).IsNil() + Expect(result).To(Equal(time.Hour + 30*time.Minute)) + }, + }), @@ Case("with decimal unit", testcase{ input: "1.5", unit: time.Second, assert: func(result time.Duration, err error) { - Expect(err).Is(as.ErrNotAnInteger) + Expect(err).Is(as.ErrNotAnIntegerOrDuration) Expect(result).To(Equal(time.Duration(0))) }, }), + Case("trims surrounding whitespace", testcase{ + input: " 1h ", + unit: time.Minute, + assert: func(result time.Duration, err error) { + Expect(err).IsNil() + Expect(result).To(Equal(time.Hour)) + }, + }), + Case("applies unit to trimmed integer", testcase{ + input: " 1 ", + unit: time.Minute, + assert: func(result time.Duration, err error) { + Expect(err).IsNil() + Expect(result).To(Equal(time.Minute)) + }, + }),Also applies to: 111-119
errors.go (1)
40-74: LGTM: FileError formatting and pointer-friendly Is.Consistent “env.FileError” prefix, inclusion of filename, and pointer/value target support are all solid.
errors_test.go (1)
104-107: LGTM: pointer-target case and FileError.Unwrap coverage.Good additions; they validate the extended Is semantics and Unwrap symmetry.
Also applies to: 116-128
parse_test.go (1)
17-21: Deduplicate LookupEnv stubbing with a small helper.Reduces repetition and centralizes restore handling.
Add helper (outside the selected hunks, near imports):
// withLookupEnv replaces internal.LookupEnv for the test and returns a restore func. func withLookupEnv(val string, ok bool) func() { return Restore(Original(&internal.LookupEnv).ReplacedBy( func(string) (string, bool) { return val, ok }, )) }Apply pattern; e.g., replace Lines 18-20:
- defer Restore(Original(&internal.LookupEnv).ReplacedBy(func(string) (string, bool) { - return "123", true - })) + defer withLookupEnv("123", true)()Repeat for the other stubs in the listed ranges.
Also applies to: 33-37, 51-54, 67-70, 83-86, 100-102, 117-119, 134-136
as/duration.go (1)
4-8: Verify/fix multi-%w fmt.Errorf usages in as/duration.goReplace any fmt.Errorf calls that use multiple %w with a single %w wrapping errors.Join(...) (add import "errors") so root causes are preserved; capture secondary parse errors and include them in errors.Join if desired. Affected: as/duration.go (imports and error-return sites around lines 4–8, 18–20, 35–46). Repo scan failed (ripgrep returned "No files were searched"); run a search for fmt.Errorf occurrences before landing the change: rg -nP 'fmt.Errorf([^)]%w[^)]%w' -S.
parse.go (3)
59-71: Parse semantics LGTM (defaults, empty-string, error wrapping).Behavior matches docs; invalid values wrap with InvalidValueError and missing var returns ParseError{ErrNotSet}.
95-97: Nil-target check is correct and fails fast.Returning ParseError{VariableName, ErrTargetIsNil} is consistent with the package’s error model.
55-57: No action required — ParseError already implements Unwrap().
errors.go defines ParseError (VariableName, Err) and an Unwrap() method returning e.Err, so errors.Is/errors.As will work.load_test.go (2)
241-248: Good transactional failure test.Verifies Setenv failures roll back all changes.
250-257: Add test: comments with leading spaces are ignored.Covers " # comment" path which code handles via TrimSpace.
Apply this diff to add a case:
@@ Case("ignores commented lines", testcase{ content: "# VAR1=value1\nVAR2=value2\n", assert: func(err error) { Expect(err).IsNil() Expect(os.Environ()).Should(HaveLen(1)) Expect(os.Getenv("VAR2")).To(Equal("value2")) }, }), + + Case("ignores commented lines with leading spaces", testcase{ + content: " # VAR1=value1\nVAR2=value2\n", + assert: func(err error) { + Expect(err).IsNil() + Expect(os.Environ()).Should(HaveLen(1)) + Expect(os.Getenv("VAR2")).To(Equal("value2")) + }, + }),as/errors.go (2)
5-16: Multi-error type is well-formed.combinedError correctly supports errors.Is/As via Unwrap() []error.
18-26: Error set reads clearly and is consistent (“URL” vs “URI”).Public error wording matches identifiers; sentinel for integer-or-duration is helpful.
load.go (1)
145-151: Scanner error handling LGTM.Appending scanner.Err() and joining preserves transactional semantics. (Previously flagged; now fixed.)
There was a problem hiding this comment.
Actionable comments posted: 11
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (6)
state.go (1)
13-17: Docstring still refers to removedResetmethodLine 16 still tells consumers to defer
Reset, but the public API now exposesRestore(). Please update the comment to point to the new method name so the docs don’t misdirect callers.as/url.go (2)
10-23: Doc/behavior mismatch: AbsoluteURL only checks scheme.Comment says “scheme, host, and path”, but the code only checks IsAbs (scheme).
-// AbsoluteURL converts a string to an absolute URI. An absolute URI is a URI -// that includes a scheme, host, and path. The function returns an error if the -// string is not an absolute URI. +// AbsoluteURL converts a string to an absolute URL. For this package, an +// absolute URL is one that includes a scheme and a host. The function returns +// an error if the string is not an absolute URL.
31-39: Also validate host to enforce “absolute URL”.Enforce host presence per the updated doc.
func AbsoluteURL(s string) (*url.URL, error) { - u, err := internal.ParseURL(s) + u, err := internal.ParseURL(s) if err != nil { return nil, err //nolint:wrapcheck // returns the parse error from [url.Parse] } - if !u.IsAbs() { + if !u.IsAbs() || u.Host == "" { return nil, env.InvalidValueError{Value: s, Err: ErrNotAnAbsoluteURL} } return u, nil }as/portNo.go (1)
3-9: Trim whitespace before parsing.Aligns with as.Duration and makes the API tolerant of benign spaces.
import ( "fmt" "math" "strconv" + "strings" "github.com/blugnu/env" )env.go (2)
56-58: Route through internal.LookupEnv for consistency and testability.Matches IsSet/Set/Unset seams and simplifies stubbing in tests.
func Lookup(name string) (string, bool) { - return os.LookupEnv(name) + return internal.LookupEnv(name) }
28-30: Optional: implement Get via internal.LookupEnv for the same seam.Keeps all env access behind internal for consistent testing.
func Get(name string) string { - return os.Getenv(name) + v, _ := internal.LookupEnv(name) + return v }
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (37)
.golangci.yml(1 hunks)README.md(4 hunks)as/as_test.go(0 hunks)as/bool.go(1 hunks)as/bool_test.go(1 hunks)as/duration.go(1 hunks)as/duration_test.go(1 hunks)as/errors.go(1 hunks)as/filename.go(1 hunks)as/filename_test.go(1 hunks)as/funcs.go(0 hunks)as/int.go(1 hunks)as/int_test.go(1 hunks)as/internal/funcs.go(1 hunks)as/portNo.go(2 hunks)as/portNo_test.go(1 hunks)as/string_test.go(1 hunks)as/url.go(2 hunks)as/url_test.go(1 hunks)env.go(4 hunks)env_test.go(1 hunks)errors.go(4 hunks)errors_test.go(1 hunks)funcs.go(0 hunks)funcs_test.go(0 hunks)go.mod(1 hunks)internal/consts.go(1 hunks)internal/funcs.go(1 hunks)internal/funcs_test.go(1 hunks)load.go(1 hunks)load_test.go(1 hunks)parse.go(2 hunks)parse_test.go(1 hunks)state.go(3 hunks)state_test.go(1 hunks)vars.go(1 hunks)vars_test.go(1 hunks)
💤 Files with no reviewable changes (4)
- funcs.go
- funcs_test.go
- as/as_test.go
- as/funcs.go
🧰 Additional context used
🧠 Learnings (15)
📚 Learning: 2024-07-04T08:27:07.225Z
Learnt from: deltics
PR: blugnu/env#2
File: load.go:63-91
Timestamp: 2024-07-04T08:27:07.225Z
Learning: When suggesting improvements for error handling, ensure to verify that all types of errors are adequately covered in the tests, including multiple errors being joined.
Applied to files:
errors_test.goas/duration_test.goas/portNo_test.go
📚 Learning: 2025-09-24T23:02:26.831Z
Learnt from: deltics
PR: blugnu/env#10
File: as/filename.go:21-29
Timestamp: 2025-09-24T23:02:26.831Z
Learning: In as/filename.go, differentiate between empty filename conditions and file-not-existing conditions using distinct sentinel errors for better semantic clarity.
Applied to files:
errors_test.goerrors.goas/filename.goload.goas/filename_test.goas/errors.gointernal/funcs.go
📚 Learning: 2025-09-21T03:03:26.651Z
Learnt from: deltics
PR: blugnu/env#10
File: state.go:65-66
Timestamp: 2025-09-21T03:03:26.651Z
Learning: In the blugnu/env codebase, test seams (like internal package wrappers around OS functions) should only be introduced where required for testing, not for consistency alone. The practice is to use seams only where needed rather than universally wrapping all OS calls.
Applied to files:
state.govars.gointernal/funcs.go
📚 Learning: 2025-09-24T23:04:07.744Z
Learnt from: deltics
PR: blugnu/env#10
File: as/duration_test.go:111-118
Timestamp: 2025-09-24T23:04:07.744Z
Learning: The ErrNotAnIntegerOrDuration error in the as package is implemented as a combinedError that allows testing for any constituent error (ErrNotAnInteger, ErrNotADuration, or strconv.ErrSyntax) using errors.Is(), providing flexible error handling for callers.
Applied to files:
as/int_test.goas/duration.goas/int.goas/duration_test.goas/errors.go
📚 Learning: 2025-09-13T00:47:52.975Z
Learnt from: deltics
PR: blugnu/env#10
File: as/int.go:3-6
Timestamp: 2025-09-13T00:47:52.975Z
Learning: fmt.Errorf supports multiple %w verbs in a single format string. When multiple %w verbs are present, the returned error implements an Unwrap method returning a []error containing all the %w operands in the order they appear in the arguments. This allows wrapping multiple errors in a single fmt.Errorf call.
Applied to files:
as/int_test.goas/int.go
📚 Learning: 2025-09-12T18:30:13.036Z
Learnt from: deltics
PR: blugnu/env#10
File: .coderabbit.yaml:4-8
Timestamp: 2025-09-12T18:30:13.036Z
Learning: CodeRabbit now supports golangci-lint v2, so separate v1 configuration files for CodeRabbit are no longer necessary. Both v1 and v2 versions are supported by CodeRabbit's tooling.
Applied to files:
.golangci.yml
📚 Learning: 2025-09-24T22:24:17.657Z
Learnt from: deltics
PR: blugnu/env#10
File: .golangci.yml:1-3
Timestamp: 2025-09-24T22:24:17.657Z
Learning: In the blugnu/env project, deltics prefers to rely on golangci-lint's automatic Go version detection from go.mod rather than explicit run.go configuration, as it's more robust and avoids version synchronization issues. The project size doesn't warrant explicit timeout configuration.
Applied to files:
.golangci.ymlgo.mod
📚 Learning: 2025-09-24T22:33:15.369Z
Learnt from: deltics
PR: blugnu/env#10
File: .golangci.yml:21-24
Timestamp: 2025-09-24T22:33:15.369Z
Learning: In the blugnu/env project, deltics prefers to keep the intrange linter disabled, favoring explicit loop constructs over int ranges for clarity in individual circumstances.
Applied to files:
.golangci.yml
📚 Learning: 2025-09-21T03:06:50.819Z
Learnt from: deltics
PR: blugnu/env#10
File: state_test.go:25-42
Timestamp: 2025-09-21T03:06:50.819Z
Learning: When testing environment state management functions, don't use those same functions as safety mechanisms in tests. This creates circular dependencies where test infrastructure relies on the system under test, potentially causing cascading failures.
Applied to files:
state_test.go
📚 Learning: 2024-07-04T09:04:46.594Z
Learnt from: deltics
PR: blugnu/env#2
File: load.go:94-121
Timestamp: 2024-07-04T09:04:46.594Z
Learning: When suggesting the use of `bufio.Scanner` for reading lines in files, consider the potential impact on testing complexity, especially if efficiency is not critical.
Applied to files:
load_test.goload.go
📚 Learning: 2025-09-24T23:02:26.831Z
Learnt from: deltics
PR: blugnu/env#10
File: as/filename.go:21-29
Timestamp: 2025-09-24T23:02:26.831Z
Learning: In the as package, prefer returning underlying filesystem errors directly rather than wrapping them with sentinel errors to avoid "sentinel for sentinel" patterns.
Applied to files:
as/filename.go
📚 Learning: 2025-09-15T03:59:10.905Z
Learnt from: deltics
PR: blugnu/env#10
File: internal/consts.go:3-6
Timestamp: 2025-09-15T03:59:10.905Z
Learning: In Go, filepath.Join cannot be used in const declarations since it's a function call. For simple relative path prefixes like "./", string concatenation is cross-platform compatible and preferable to using vars with filepath.Join.
Applied to files:
internal/consts.go
📚 Learning: 2025-09-21T02:35:04.766Z
Learnt from: deltics
PR: blugnu/env#10
File: load.go:163-171
Timestamp: 2025-09-21T02:35:04.766Z
Learning: The default bufio.Scanner uses ScanLines which automatically removes both \r and \n characters when reading lines, so manual CRLF handling is not needed when using scanner.Scan().
Applied to files:
load.go
📚 Learning: 2024-07-04T08:06:24.732Z
Learnt from: deltics
PR: blugnu/env#2
File: load_test.go:3-10
Timestamp: 2024-07-04T08:06:24.732Z
Learning: The `io/fs` package is used in the `load_test.go` file, specifically `fs.ErrNotExist`.
Applied to files:
load.goas/filename_test.gointernal/funcs_test.gointernal/funcs.go
📚 Learning: 2025-09-24T23:20:43.209Z
Learnt from: deltics
PR: blugnu/env#10
File: internal/func_test.go:16-18
Timestamp: 2025-09-24T23:20:43.209Z
Learning: In tests, prefer using actual values that consumers would use (like ".env") rather than internal constants, even when those constants exist. Tests should reflect real-world usage patterns, not implementation details.
Applied to files:
internal/funcs_test.go
🧬 Code graph analysis (26)
errors_test.go (1)
errors.go (5)
FileError(28-31)NewFileError(33-38)ParseError(84-87)InvalidValueError(148-151)RangeError(213-216)
state.go (1)
internal/funcs.go (1)
Setenv(21-21)
as/url_test.go (2)
as/url.go (1)
AbsoluteURL(31-40)as/errors.go (1)
ErrNotAnAbsoluteURL(32-32)
as/internal/funcs.go (1)
parse.go (1)
Parse(54-72)
as/int_test.go (2)
as/int.go (1)
Int(19-25)as/errors.go (1)
ErrNotAnInteger(29-29)
as/duration.go (1)
as/errors.go (1)
ErrNotADuration(30-30)
vars_test.go (2)
state.go (1)
State(38-40)vars.go (1)
Vars(29-60)
state_test.go (1)
state.go (1)
State(38-40)
vars.go (1)
internal/funcs.go (1)
LookupEnv(15-15)
as/url.go (1)
as/internal/funcs.go (1)
ParseURL(7-7)
load_test.go (5)
internal/funcs.go (5)
FileExists(31-37)NewFileReader(50-52)Setenv(21-21)LookupEnv(15-15)ScanLines(18-18)state.go (1)
State(38-40)load.go (3)
Load(32-71)LoadFile(83-99)LoadFromReader(109-155)internal/consts.go (1)
EnvFile(6-6)errors.go (2)
FileError(28-31)ErrInvalidEntry(11-11)
parse_test.go (3)
internal/funcs.go (1)
LookupEnv(15-15)parse.go (2)
Parse(54-72)ParseInto(95-107)errors.go (4)
ErrNotSet(15-15)ParseError(84-87)InvalidValueError(148-151)ErrTargetIsNil(22-22)
as/string_test.go (1)
as/string.go (1)
String(14-16)
errors.go (1)
as/filename.go (1)
Filename(9-28)
as/filename.go (1)
as/errors.go (2)
ErrFilenameIsEmpty(27-27)ErrFilenameIsDirectory(28-28)
env.go (1)
internal/funcs.go (3)
LookupEnv(15-15)Setenv(21-21)Unsetenv(24-24)
as/int.go (1)
as/errors.go (1)
ErrNotAnInteger(29-29)
as/bool_test.go (1)
as/bool.go (1)
Bool(15-28)
as/portNo.go (1)
errors.go (1)
RangeError(213-216)
parse.go (2)
errors.go (4)
ParseError(84-87)InvalidValueError(148-151)ErrNotSet(15-15)ErrTargetIsNil(22-22)internal/funcs.go (1)
LookupEnv(15-15)
load.go (4)
state.go (1)
State(38-40)internal/consts.go (1)
EnvFile(6-6)internal/funcs.go (5)
FileExists(31-37)NewFileReader(50-52)ScanLines(18-18)LookupEnv(15-15)Setenv(21-21)errors.go (3)
NewFileError(33-38)ErrInvalidEntry(11-11)ErrSetFailed(19-19)
as/filename_test.go (2)
as/filename.go (1)
Filename(9-28)as/errors.go (2)
ErrFilenameIsEmpty(27-27)ErrFilenameIsDirectory(28-28)
as/duration_test.go (2)
as/duration.go (2)
Duration(14-21)DurationIn(32-50)as/errors.go (3)
ErrNotADuration(30-30)ErrNotAnInteger(29-29)ErrNotAnIntegerOrDuration(31-31)
internal/funcs_test.go (1)
internal/funcs.go (3)
FileExists(31-37)Open(12-12)NewFileReader(50-52)
env_test.go (3)
state.go (1)
State(38-40)internal/funcs.go (2)
Setenv(21-21)Unsetenv(24-24)env.go (6)
Clear(11-13)IsSet(35-38)Get(28-30)Lookup(56-58)Set(74-76)Unset(91-98)
as/portNo_test.go (2)
as/portNo.go (1)
PortNo(31-45)errors.go (1)
RangeError(213-216)
🪛 GitHub Check: release / qa / run tests
as/internal/funcs.go
[failure] 6-6:
godoc should start with symbol name ("ParseURL") (godoclint)
internal/funcs.go
[failure] 23-23:
godoc should start with symbol name ("Unsetenv") (godoclint)
[failure] 20-20:
godoc should start with symbol name ("Setenv") (godoclint)
[failure] 17-17:
godoc should start with symbol name ("ScanLines") (godoclint)
[failure] 14-14:
godoc should start with symbol name ("LookupEnv") (godoclint)
[failure] 11-11:
godoc should start with symbol name ("Open") (godoclint)
[failure] 6-6:
godoc should start with symbol name ("ParseURL") (godoclint)
🪛 GitHub Actions: release
as/internal/funcs.go
[error] 6-6: godoc should start with symbol name ("ParseURL") (godoclint)
go.mod
[error] 1-1: Script error: file does not exist (./go.mod)
internal/funcs.go
[error] 6-6: godoc should start with symbol name ("ParseURL") (godoclint)
🔇 Additional comments (30)
as/bool.go (1)
9-14: Polish the Bool doc comment grammar.The sentence uses “an Boolean” and double spacing. Tidy this to keep the package docs clean, e.g.:
-// Bool converts a string to an Boolean. Conversion is initially attempted +// Bool converts a string to a Boolean. Conversion is initially attemptedas/bool_test.go (1)
28-66: Broad boolean normalization coverage looks great.Thanks for adding the mixed‑case and whitespace tokens—this aligns perfectly with the lowercasing + TrimSpace behavior in
as.Bool.as/filename.go (1)
14-25: Solid filename validation guards.The explicit empty-string and directory checks tighten the contract while still surfacing the original
os.Staterrors when appropriate.as/url_test.go (1)
25-47: Happy path and failure modes are well covered.Exercising absolute, relative, and malformed URLs (including the type assertion on the parse error) gives us high confidence in
AbsoluteURL.state_test.go (1)
25-41: Restore flow verification looks good.Clearing the environment and asserting both variables post-
Restore()confirms the renamed API behaves as intended.as/errors.go (1)
7-32: Nice multi-error unwrapping utility.
notAnIntegerOrDurationErrorprovides the expectederrors.Iscoverage across the new sentinels without sacrificing readability.errors_test.go (1)
130-540: Error semantics are thoroughly exercised.The expanded tables—especially the pointer-target cases—lock down
Is/Unwrapbehavior for all custom error types.parse.go (1)
54-106: Helpful Parse/ParseInto refinements.The updated docs read cleanly, and the
niltarget guard with aParseErrorsentinel makesParseIntosafer to consume.as/int.go (1)
12-23: Clearer conversion failure signal.The added
%wcontext ensures callers can detectErrNotAnIntegerwhile still seeing the underlyingstrconverror.env_test.go (1)
24-26: Good move to black‑box via env.IsSet and per‑test Restore.Clean isolation and public API usage. Looks solid.
as/filename_test.go (1)
37-43: Platform‑robust invalid/absent filename checks.Assertions avoid OS‑specific errno brittleness while still validating behavior. LGTM.
as/duration.go (2)
14-21: LGTM: trims input before parsing Duration.Whitespace tolerance is user‑friendly and low risk.
3-8: Fix: multiple %w in fmt.Errorf causes a runtime panic.Only one %w is allowed; use errors.Join to wrap multiple causes.
import ( + "errors" "fmt" "strconv" "strings" "time" )⛔ Skipped due to learnings
Learnt from: deltics PR: blugnu/env#10 File: as/int.go:3-6 Timestamp: 2025-09-13T00:47:52.975Z Learning: fmt.Errorf supports multiple %w verbs in a single format string. When multiple %w verbs are present, the returned error implements an Unwrap method returning a []error containing all the %w operands in the order they appear in the arguments. This allows wrapping multiple errors in a single fmt.Errorf call.parse_test.go (1)
10-12: DRY the repeated LookupEnv stubs with a tiny helper.Reduces repetition and centralizes restore handling.
Apply this diff to add a helper:
import ( "errors" "strconv" "testing" . "github.com/blugnu/test" "github.com/blugnu/env" "github.com/blugnu/env/internal" ) +// withLookupEnv replaces internal.LookupEnv for the duration of the test, returning a restore func. +func withLookupEnv(val string, ok bool) func() { + return Restore(Original(&internal.LookupEnv).ReplacedBy( + func(string) (string, bool) { return val, ok }, + )) +}Then replace repeated blocks, e.g.:
- defer Restore(Original(&internal.LookupEnv).ReplacedBy(func(string) (string, bool) { - return "123", true - })) + defer withLookupEnv("123", true)()- defer Restore(Original(&internal.LookupEnv).ReplacedBy(func(string) (string, bool) { - return "", false - })) + defer withLookupEnv("", false)()go.mod (1)
3-6: Pin toolchain and fix release workflow working directory.
- Add toolchain to align local/CI builds.
- The release failure “file does not exist (./go.mod)” indicates the job isn’t running at repo root or didn’t checkout the code.
module github.com/blugnu/env go 1.23 +toolchain go1.23.0 require github.com/blugnu/test v0.12.0Action items:
- Ensure the release job includes actions/checkout and runs from the repo root (or sets working-directory to the repo root) before running scripts.
as/string_test.go (1)
11-23: LGTM: minimal, clear test..golangci.yml (1)
1-51: LGTM: config matches stated project preferences.Defaults + selective disables look intentional; intrange remains disabled per preference; copyloopvar not disabled.
as/int_test.go (1)
13-43: LGTM: good coverage including error chain assertions.internal/funcs_test.go (2)
12-18: LGTM: consumer-oriented filename choice and positive/negative checks.
20-56: LGTM: Open patched via indirection; both success/failure paths verified.load_test.go (2)
258-275: Add test: comment lines with leading spaces are ignored.You already cover “# …”; add “ # …” to lock behavior. This was previously requested.
@@ - Case("ignores commented lines", testcase{ + Case("ignores commented lines", testcase{ content: "# VAR1=value1\nVAR2=value2\n", assert: func(err error) { Expect(err).IsNil() Expect(os.Environ()).Should(HaveLen(1)) Expect(os.Getenv("VAR2")).To(Equal("value2")) }, }), + + Case("ignores commented lines with leading spaces", testcase{ + content: " # VAR1=value1\nVAR2=value2\n", + assert: func(err error) { + Expect(err).IsNil() + Expect(os.Environ()).Should(HaveLen(1)) + Expect(os.Getenv("VAR2")).To(Equal("value2")) + }, + }),
112-121: Add test: pre-set empty variables are not overwritten.We still don’t cover the “existing but empty” case. Suggest a small standalone test to avoid altering this table’s arrange step.
I can add a new Test ensuring a pre-set empty variable remains empty when the file provides a value. Want me to push the test code?
vars_test.go (1)
26-56: Good coverage and state hygiene.Cases exercise all/selected/trimmed paths and use Restore/T().Setenv correctly.
load.go (2)
13-71: LGTM: transactional load, .env precedence, and error aggregation.Dedup + existence filtering, per-file FileError context, and full rollback on any error are solid.
110-155: LGTM: reader-based transactional core with strict entry validation.Scanner error capture, invalid-entry handling, non-overwrite semantics, and contextual wrapping are correct.
as/duration_test.go (1)
82-121: Add missing edge cases: decimals with unit and whitespace handling.Per prior discussion on combinedError and tolerant parsing, add “1.5h” and whitespace cases.
// MARK: edge cases @@ Case("with negative duration expression", testcase{ input: "-2h30m", unit: time.Minute, assert: func(result time.Duration, err error) { Expect(err).IsNil() Expect(result).To(Equal(-2*time.Hour - 30*time.Minute)) }, }), + Case("with decimal duration expression", testcase{ + input: "1.5h", + unit: time.Second, // unit ignored for valid expressions + assert: func(result time.Duration, err error) { + Expect(err).IsNil() + Expect(result).To(Equal(90 * time.Minute)) + }, + }), + Case("with decimal unit", testcase{ input: "1.5", unit: time.Second, assert: func(result time.Duration, err error) { Expect(err).Is(as.ErrNotADuration) Expect(err).Is(as.ErrNotAnInteger) Expect(err).Is(as.ErrNotAnIntegerOrDuration) Expect(result).To(Equal(time.Duration(0))) }, }), + + Case("trims whitespace in input (expression)", testcase{ + input: " 1h ", + unit: time.Second, + assert: func(result time.Duration, err error) { + Expect(err).IsNil() + Expect(result).To(Equal(time.Hour)) + }, + }), + + Case("trims whitespace in input (plain integer applies unit)", testcase{ + input: " 1 ", + unit: time.Second, + assert: func(result time.Duration, err error) { + Expect(err).IsNil() + Expect(result).To(Equal(1 * time.Second)) + }, + }),as/portNo_test.go (2)
46-49: Strengthen upper-bound assertions to verify RangeError bounds.Mirror the “-1” case: assert Min/Max via errors.As to lock the 0..65535 contract.
- Case("65536", func(result int, err error) { - Expect(err).Is(env.RangeError[int]{}) - Expect(result).To(Equal(0)) - }), + Case("65536", func(result int, err error) { + var rangeErr env.RangeError[int] + Expect(errors.As(err, &rangeErr)).To(BeTrue()) + Expect(rangeErr.Min).To(Equal(0)) + Expect(rangeErr.Max).To(Equal(int(math.MaxUint16))) + Expect(result).To(Equal(0)) + }),
23-29: Use math.MaxUint16 for clarity and future‑proofing.Avoid hard‑coding 65535; assert against int(math.MaxUint16) to mirror implementation.
- Expect(rangeErr.Max).To(Equal(65535)) + Expect(rangeErr.Max).To(Equal(int(math.MaxUint16)))Also add the missing import:
import ( "errors" + "math" "strconv" "testing"README.md (2)
54-61: Revised Parse example looks solid.The snippet now compiles cleanly and demonstrates default handling without shadowing issues. 👍
16-16: go.mod matches README badge version – go.mod declares go 1.23, matching the “Go ≥ 1.23” badge.
There was a problem hiding this comment.
Actionable comments posted: 10
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (5)
errors.go (2)
140-143: Fix ParseError.Unwrap doc string.The comment implies it unwraps an env.Parse call rather than the ParseError itself.
Apply:
-// Unwrap returns the error that caused the env.Parse. +// Unwrap returns the error that caused the ParseError.
218-225: Align RangeError.Error docs with actual output.Docs say “out of range...” while the implementation prefixes with “env.RangeError...”.
Apply:
-// Error returns a string representation of the error in the form: -// -// out of range: <Min> <= (x) <= <Max> -// -// If Min and Max are both the zero value of T: -// -// out of range +// Error returns a string representation of the error in the form: +// +// env.RangeError: <Min> <= (x) <= <Max> +// +// If Min and Max are both the zero value of T: +// +// env.RangeErroras/url.go (1)
10-13: Doc/behavior mismatch for “absolute” URL.u.IsAbs() only requires a non-empty scheme; current docs claim scheme, host, and path. Either tighten the check (require u.Host) or fix docs. Recommending doc update to match net/url semantics.
Apply:
-// AbsoluteURL converts a string to an absolute URI. An absolute URI is a URI -// that includes a scheme, host, and path. The function returns an error if the -// string is not an absolute URI. +// AbsoluteURL converts a string to an absolute URL. A URL is considered absolute +// if it has a non-empty scheme per [url.URL.IsAbs]. The function returns an error +// if the string is not an absolute URL. @@ -// - if the URL is not an absolute URI, the function returns an -// env.InvalidValueError +// - if the URL is not absolute, the function returns an env.InvalidValueErrorAlso applies to: 29-31
state.go (1)
13-37: Update docs/examples to use Restore (not Reset).Top-level State docs still reference Reset and show it in the example.
Apply:
-// the test has run (by deferring a call to the Reset method on the +// the test has run (by deferring a call to the Restore method on the // returned state value). @@ -// defer env.State().Reset() +// defer env.State().Restore()env.go (1)
56-58: Route Lookup through internal.LookupEnv for consistency/testability.Aligns with IsSet and keeps a single seam.
-func Lookup(name string) (string, bool) { - return os.LookupEnv(name) -} +func Lookup(name string) (string, bool) { + return internal.LookupEnv(name) +}
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (34)
.golangci.yml(1 hunks)README.md(4 hunks)as/as_test.go(0 hunks)as/bool_test.go(1 hunks)as/duration.go(1 hunks)as/duration_test.go(1 hunks)as/filename_test.go(1 hunks)as/funcs.go(0 hunks)as/int.go(1 hunks)as/int_test.go(1 hunks)as/internal/funcs.go(1 hunks)as/portNo.go(2 hunks)as/portNo_test.go(1 hunks)as/string_test.go(1 hunks)as/url.go(2 hunks)as/url_test.go(1 hunks)env.go(4 hunks)env_test.go(1 hunks)errors.go(4 hunks)errors_test.go(1 hunks)funcs.go(0 hunks)funcs_test.go(0 hunks)go.mod(1 hunks)internal/consts.go(1 hunks)internal/funcs.go(1 hunks)internal/funcs_test.go(1 hunks)load.go(1 hunks)load_test.go(1 hunks)parse.go(2 hunks)parse_test.go(1 hunks)state.go(3 hunks)state_test.go(1 hunks)vars.go(1 hunks)vars_test.go(1 hunks)
💤 Files with no reviewable changes (4)
- funcs_test.go
- as/as_test.go
- funcs.go
- as/funcs.go
🧰 Additional context used
🧠 Learnings (14)
📚 Learning: 2025-09-24T23:04:07.744Z
Learnt from: deltics
PR: blugnu/env#10
File: as/duration_test.go:111-118
Timestamp: 2025-09-24T23:04:07.744Z
Learning: The ErrNotAnIntegerOrDuration error in the as package is implemented as a combinedError that allows testing for any constituent error (ErrNotAnInteger, ErrNotADuration, or strconv.ErrSyntax) using errors.Is(), providing flexible error handling for callers.
Applied to files:
as/duration_test.goas/int.goas/int_test.goas/duration.go
📚 Learning: 2024-07-04T08:27:07.225Z
Learnt from: deltics
PR: blugnu/env#2
File: load.go:63-91
Timestamp: 2024-07-04T08:27:07.225Z
Learning: When suggesting improvements for error handling, ensure to verify that all types of errors are adequately covered in the tests, including multiple errors being joined.
Applied to files:
as/duration_test.goas/portNo_test.goerrors_test.go
📚 Learning: 2025-09-13T00:47:52.975Z
Learnt from: deltics
PR: blugnu/env#10
File: as/int.go:3-6
Timestamp: 2025-09-13T00:47:52.975Z
Learning: fmt.Errorf supports multiple %w verbs in a single format string. When multiple %w verbs are present, the returned error implements an Unwrap method returning a []error containing all the %w operands in the order they appear in the arguments. This allows wrapping multiple errors in a single fmt.Errorf call.
Applied to files:
as/int.goas/int_test.goas/duration.go
📚 Learning: 2025-09-21T03:03:26.651Z
Learnt from: deltics
PR: blugnu/env#10
File: state.go:65-66
Timestamp: 2025-09-21T03:03:26.651Z
Learning: In the blugnu/env codebase, test seams (like internal package wrappers around OS functions) should only be introduced where required for testing, not for consistency alone. The practice is to use seams only where needed rather than universally wrapping all OS calls.
Applied to files:
state.gointernal/funcs.govars.go
📚 Learning: 2024-07-04T09:04:46.594Z
Learnt from: deltics
PR: blugnu/env#2
File: load.go:94-121
Timestamp: 2024-07-04T09:04:46.594Z
Learning: When suggesting the use of `bufio.Scanner` for reading lines in files, consider the potential impact on testing complexity, especially if efficiency is not critical.
Applied to files:
load_test.goload.go
📚 Learning: 2025-09-21T03:06:50.819Z
Learnt from: deltics
PR: blugnu/env#10
File: state_test.go:25-42
Timestamp: 2025-09-21T03:06:50.819Z
Learning: When testing environment state management functions, don't use those same functions as safety mechanisms in tests. This creates circular dependencies where test infrastructure relies on the system under test, potentially causing cascading failures.
Applied to files:
state_test.go
📚 Learning: 2025-09-24T22:24:17.657Z
Learnt from: deltics
PR: blugnu/env#10
File: .golangci.yml:1-3
Timestamp: 2025-09-24T22:24:17.657Z
Learning: In the blugnu/env project, deltics prefers to rely on golangci-lint's automatic Go version detection from go.mod rather than explicit run.go configuration, as it's more robust and avoids version synchronization issues. The project size doesn't warrant explicit timeout configuration.
Applied to files:
go.mod.golangci.yml
📚 Learning: 2025-09-24T23:02:26.831Z
Learnt from: deltics
PR: blugnu/env#10
File: as/filename.go:21-29
Timestamp: 2025-09-24T23:02:26.831Z
Learning: In as/filename.go, differentiate between empty filename conditions and file-not-existing conditions using distinct sentinel errors for better semantic clarity.
Applied to files:
internal/funcs.goload.goerrors.goerrors_test.goas/filename_test.go
📚 Learning: 2024-07-04T08:06:24.732Z
Learnt from: deltics
PR: blugnu/env#2
File: load_test.go:3-10
Timestamp: 2024-07-04T08:06:24.732Z
Learning: The `io/fs` package is used in the `load_test.go` file, specifically `fs.ErrNotExist`.
Applied to files:
internal/funcs.goload.goas/filename_test.gointernal/funcs_test.go
📚 Learning: 2025-09-15T03:59:10.905Z
Learnt from: deltics
PR: blugnu/env#10
File: internal/consts.go:3-6
Timestamp: 2025-09-15T03:59:10.905Z
Learning: In Go, filepath.Join cannot be used in const declarations since it's a function call. For simple relative path prefixes like "./", string concatenation is cross-platform compatible and preferable to using vars with filepath.Join.
Applied to files:
internal/consts.go
📚 Learning: 2025-09-21T02:35:04.766Z
Learnt from: deltics
PR: blugnu/env#10
File: load.go:163-171
Timestamp: 2025-09-21T02:35:04.766Z
Learning: The default bufio.Scanner uses ScanLines which automatically removes both \r and \n characters when reading lines, so manual CRLF handling is not needed when using scanner.Scan().
Applied to files:
load.go
📚 Learning: 2025-09-12T18:30:13.036Z
Learnt from: deltics
PR: blugnu/env#10
File: .coderabbit.yaml:4-8
Timestamp: 2025-09-12T18:30:13.036Z
Learning: CodeRabbit now supports golangci-lint v2, so separate v1 configuration files for CodeRabbit are no longer necessary. Both v1 and v2 versions are supported by CodeRabbit's tooling.
Applied to files:
.golangci.yml
📚 Learning: 2025-09-24T22:33:15.369Z
Learnt from: deltics
PR: blugnu/env#10
File: .golangci.yml:21-24
Timestamp: 2025-09-24T22:33:15.369Z
Learning: In the blugnu/env project, deltics prefers to keep the intrange linter disabled, favoring explicit loop constructs over int ranges for clarity in individual circumstances.
Applied to files:
.golangci.yml
📚 Learning: 2025-09-24T23:20:43.209Z
Learnt from: deltics
PR: blugnu/env#10
File: internal/func_test.go:16-18
Timestamp: 2025-09-24T23:20:43.209Z
Learning: In tests, prefer using actual values that consumers would use (like ".env") rather than internal constants, even when those constants exist. Tests should reflect real-world usage patterns, not implementation details.
Applied to files:
internal/funcs_test.go
🧬 Code graph analysis (25)
as/duration_test.go (2)
as/duration.go (2)
Duration(14-21)DurationIn(32-50)as/errors.go (3)
ErrNotADuration(30-30)ErrNotAnInteger(29-29)ErrNotAnIntegerOrDuration(31-31)
as/portNo.go (1)
errors.go (1)
RangeError(213-216)
as/string_test.go (1)
as/string.go (1)
String(14-16)
as/url.go (1)
as/internal/funcs.go (1)
ParseURL(7-7)
as/int.go (1)
as/errors.go (1)
ErrNotAnInteger(29-29)
state.go (1)
internal/funcs.go (1)
Setenv(22-22)
load_test.go (5)
internal/funcs.go (5)
FileExists(38-48)NewFileReader(61-63)Setenv(22-22)LookupEnv(16-16)ScanLines(19-19)state.go (1)
State(38-40)load.go (3)
Load(32-71)LoadFile(83-99)LoadFromReader(109-155)internal/consts.go (1)
EnvFile(4-4)errors.go (2)
FileError(28-31)ErrInvalidEntry(11-11)
state_test.go (1)
state.go (1)
State(38-40)
as/int_test.go (2)
as/int.go (1)
Int(19-25)as/errors.go (1)
ErrNotAnInteger(29-29)
vars_test.go (2)
state.go (1)
State(38-40)vars.go (1)
Vars(29-60)
load.go (4)
state.go (1)
State(38-40)internal/consts.go (1)
EnvFile(4-4)internal/funcs.go (5)
FileExists(38-48)NewFileReader(61-63)ScanLines(19-19)LookupEnv(16-16)Setenv(22-22)errors.go (3)
NewFileError(33-38)ErrInvalidEntry(11-11)ErrSetFailed(19-19)
errors.go (1)
as/filename.go (1)
Filename(9-28)
as/duration.go (1)
as/errors.go (1)
ErrNotADuration(30-30)
env_test.go (3)
state.go (1)
State(38-40)internal/funcs.go (2)
Setenv(22-22)Unsetenv(28-28)env.go (6)
Clear(11-13)IsSet(35-38)Get(28-30)Lookup(56-58)Set(74-76)Unset(91-98)
vars.go (1)
internal/funcs.go (1)
LookupEnv(16-16)
as/portNo_test.go (2)
as/portNo.go (1)
PortNo(32-47)errors.go (1)
RangeError(213-216)
as/bool_test.go (1)
as/bool.go (1)
Bool(15-28)
as/internal/funcs.go (1)
parse.go (1)
Parse(54-72)
errors_test.go (1)
errors.go (5)
FileError(28-31)NewFileError(33-38)ParseError(84-87)InvalidValueError(148-151)RangeError(213-216)
parse_test.go (3)
internal/funcs.go (1)
LookupEnv(16-16)parse.go (2)
Parse(54-72)ParseInto(95-107)errors.go (4)
ErrNotSet(15-15)ParseError(84-87)InvalidValueError(148-151)ErrTargetIsNil(22-22)
as/url_test.go (2)
as/url.go (1)
AbsoluteURL(31-40)as/errors.go (1)
ErrNotAnAbsoluteURL(32-32)
as/filename_test.go (2)
as/filename.go (1)
Filename(9-28)as/errors.go (2)
ErrFilenameIsEmpty(27-27)ErrFilenameIsDirectory(28-28)
env.go (1)
internal/funcs.go (3)
LookupEnv(16-16)Setenv(22-22)Unsetenv(28-28)
parse.go (2)
errors.go (4)
ParseError(84-87)InvalidValueError(148-151)ErrNotSet(15-15)ErrTargetIsNil(22-22)internal/funcs.go (1)
LookupEnv(16-16)
internal/funcs_test.go (1)
internal/funcs.go (4)
FileExists(38-48)Stat(25-25)Open(13-13)NewFileReader(61-63)
🔇 Additional comments (37)
errors_test.go (1)
235-262: Pointer-target cases keeperrors.Isbehavior predictable.Great to see the
*env.ParseErrorscenarios covered—this aligns the pointer semantics with the value cases and protects future changes.as/portNo.go (1)
37-43: Whitespace trim + wrapped range errors look solid.The TrimSpace before Atoi plus the contextual wrapping keeps the converter robust while still surfacing the original error for callers. 👍
as/portNo_test.go (1)
23-54: Great job locking the RangeError details in tests.Asserting the Min/Max bounds and the ErrSyntax path gives strong confidence this helper stays tight on its contract.
as/filename_test.go (1)
29-68: LGTM: solid, platform-robust checks.Good coverage including empty, directory, non-existent, and invalid-name cases with OS-agnostic assertions.
vars.go (1)
10-59: LGTM: clear API and trimming semantics.Implementation and docs align (trimmed keys, values preserved). Sensible use of os.Environ and internal.LookupEnv.
vars_test.go (1)
12-56: LGTM: good coverage including name trimming.Tests exercise both modes and whitespace in names; setup/restoration looks correct.
as/duration.go (2)
10-21: LGTM: strict duration parsing with clear sentinel.Trimming input and wrapping both the sentinel and parse error is appropriate.
23-51: LGTM: integer-units parser with duration fallback.Behavior and error composition (combined sentinel + cause) look good; trimming handled.
internal/funcs.go (2)
31-48: Docs and behavior for FileExists are now aligned—nice.Explicit ErrNotExist check plus fi!=nil path handles edge OS behaviors.
If godoclint still flags this section, please confirm the comment lines wrap at or before 120 chars per your linter config.
61-63: Mark NewFileReader as an intentional global seam.Silence gochecknoglobals consistently with the var block above.
Apply:
-var NewFileReader = func(path string) (io.ReadCloser, error) { +//nolint:gochecknoglobals // test seam +var NewFileReader = func(path string) (io.ReadCloser, error) { return Open(path) }internal/funcs_test.go (2)
22-59: LGTM: FileExists scenarios well covered.Good use of the Stat seam to model edge cases.
62-98: LGTM: NewFileReader seam tests are concise and correct.Resource is closed; success and failure paths verified.
as/duration_test.go (1)
82-121: Add tests for “decimal with unit” and whitespace in DurationIn.Strengthens coverage for documented behaviors.
Apply:
// MARK: edge cases @@ Case("with decimal unit", testcase{ input: "1.5", unit: time.Second, assert: func(result time.Duration, err error) { Expect(err).Is(as.ErrNotADuration) Expect(err).Is(as.ErrNotAnInteger) Expect(err).Is(as.ErrNotAnIntegerOrDuration) Expect(result).To(Equal(time.Duration(0))) }, }), + + Case("with decimal value and duration unit specified in input", testcase{ + input: "1.5h", + unit: time.Second, // ignored when input is a duration expression + assert: func(result time.Duration, err error) { + Expect(err).IsNil() + Expect(result).To(Equal(90 * time.Minute)) + }, + }), + + Case("trims whitespace in integer input", testcase{ + input: " 1 ", + unit: time.Minute, + assert: func(result time.Duration, err error) { + Expect(err).IsNil() + Expect(result).To(Equal(1 * time.Minute)) + }, + }), + + Case("trims whitespace in duration expression input", testcase{ + input: " 2h30m ", + unit: time.Second, + assert: func(result time.Duration, err error) { + Expect(err).IsNil() + Expect(result).To(Equal(2*time.Hour + 30*time.Minute)) + }, + }),errors.go (6)
40-52: FileError message format is now consistent.Prefix and filename are included; aligns with other env.* errors.
54-74: FileError.Is correctly supports value and pointer targets.Matching logic and errors.Is delegation look solid.
124-138: ParseError.Is: pointer/value target support is correct.Matching semantics (empty name/err as wildcards) are preserved.
187-201: InvalidValueError.Is: pointer/value target support is correct.Works with errors.Is and maintains intended wildcard semantics.
239-256: RangeError[T].Is: generic pointer/value matching is correct.Zero-value Min/Max used as wildcards is clear and efficient.
9-23: Error docs reference valid exported APIs Load, LoadFile, LoadFromReader, Parse, and ParseInto exist and no stale LoadFiles references remain.env_test.go (2)
24-26: Tests now use public API (env.IsSet) for assertions.Good black-box coverage; avoids coupling to internals.
136-149: Error-path test for Unset stubs internal.Unsetenv correctly.Asserts errors.Is behavior and non-mutation on failure; solid.
as/string_test.go (1)
11-23: Simple, clear golden-path test.Matches as.String contract and test harness conventions.
state.go (1)
61-66: Direct os.Setenv use is acceptable here.Consistent with the “test seams only where needed” practice for this repo.
parse_test.go (3)
41-45: Strong error composition assertions — LGTM.Covers ErrNotSet, ParseError wrapping, and InvalidValueError shapes consistently.
Also applies to: 109-113, 146-149, 158-159
8-12: Deduplicate LookupEnv stubbing with a small helper.Reduces repetition and centralizes restore handling.
Add a helper after imports:
@@ "github.com/blugnu/env" "github.com/blugnu/env/internal" ) +// withLookupEnv replaces internal.LookupEnv for the test and returns a restore func. +func withLookupEnv(val string, ok bool) func() { + return Restore(Original(&internal.LookupEnv).ReplacedBy( + func(string) (string, bool) { return val, ok }, + )) +}Use it in tests (apply similarly to other occurrences shown in this file):
@@ - defer Restore(Original(&internal.LookupEnv).ReplacedBy(func(string) (string, bool) { - return "123", true - })) + defer withLookupEnv("123", true)() @@ - defer Restore(Original(&internal.LookupEnv).ReplacedBy(func(string) (string, bool) { - return "", false - })) + defer withLookupEnv("", false)() @@ - defer Restore(Original(&internal.LookupEnv).ReplacedBy(func(string) (string, bool) { - return "not-a-number", true - })) + defer withLookupEnv("not-a-number", true)() @@ - defer Restore(Original(&internal.LookupEnv).ReplacedBy(func(string) (string, bool) { - return "", false - })) + defer withLookupEnv("", false)() @@ - defer Restore(Original(&internal.LookupEnv).ReplacedBy(func(string) (string, bool) { - return "123", true - })) + defer withLookupEnv("123", true)() @@ - defer Restore(Original(&internal.LookupEnv).ReplacedBy(func(string) (string, bool) { - return "abc", true - })) + defer withLookupEnv("abc", true)() @@ - defer Restore(Original(&internal.LookupEnv).ReplacedBy(func(string) (string, bool) { - return "123", true - })) + defer withLookupEnv("123", true)() @@ - defer Restore(Original(&internal.LookupEnv).ReplacedBy(func(string) (string, bool) { - return "not a number", true - })) + defer withLookupEnv("not a number", true)()Also applies to: 18-20, 34-36, 52-54, 70-72, 86-88, 102-104, 121-123, 138-140
59-63: Assert the exact InvalidValueError inner error in conversion-fails case.Increases signal by verifying Err equals converr.
// assert Expect(err).Is(converr) Expect(err).Is(env.ParseError{VariableName: "NOT_A_NUMBER"}) - Expect(err).Is(env.InvalidValueError{Value: "not-a-number"}) + Expect(err).Is(env.InvalidValueError{Value: "not-a-number", Err: converr}) Expect(value).To(Equal(0))env.go (1)
32-38: IsSet/Set/Unset refactor — LGTM.Good move to internal seams and consistent error prefixing in Unset.
Also applies to: 73-76, 92-95
load_test.go (2)
58-77: Comprehensive coverage of Load behavior — LGTM.*Good transactional semantics checks, precedence, non-overwrite, and scanner/set errors.
Also applies to: 79-89, 100-110, 112-121, 152-167, 169-180, 219-228, 258-265, 267-275
258-265: Add a case for comment lines with leading spaces.Confirms TrimSpace-before-‘#’ handling.
Case("ignores commented lines", testcase{ content: "# VAR1=value1\nVAR2=value2\n", assert: func(err error) { Expect(err).IsNil() Expect(os.Environ()).Should(HaveLen(1)) Expect(os.Getenv("VAR2")).To(Equal("value2")) }, }), + Case("ignores commented lines with leading spaces", testcase{ + content: " # c1\n\t# c2\nVAR=ok\n", + assert: func(err error) { + Expect(err).IsNil() + Expect(os.Getenv("VAR")).To(Equal("ok")) + }, + }),README.md (1)
16-17: Approve docs changes — go.mod declares Go 1.23, matching the badge and examples.load.go (3)
13-32: Docs align with behavior; good clarity on atomicity and non-overwrite.
Comment updates about missing files, .env precedence, non-overwrite semantics, and transactional restore are consistent with the implementation.
56-71: All-or-nothing rollback across files looks correct.
Collecting all file errors via errors.Join and restoring initial state preserves atomicity and gives callers complete error context.Please confirm this all-or-nothing behavior is intended even when only one of several files fails; successful loads from earlier files will be rolled back.
117-151: Scanner error handling and per-file atomicity: LGTM.
Appending scanner.Err() and routing through handleError maintains the transactional guarantee for the reader.as/int.go (1)
12-23: Error wrapping with multiple %w is correct here.
Maintains both ErrNotAnInteger and the underlying error; tests assert both.as/internal/funcs.go (1)
5-8: ParseURL naming and doc pass lint; seam is clear.
Identifier uses proper initialism and the doc starts with the name; suitable for testing overrides.as/int_test.go (1)
22-41: Coverage and assertions look solid.
Validates bounds, zero, and error cases; asserts both strconv.ErrSyntax and as.ErrNotAnInteger.go.mod (1)
3-5: Pin toolchain to stabilize CI environment.
Add a toolchain directive to align local/CI builds with Go 1.23.Apply:
module github.com/blugnu/env go 1.23 +toolchain go1.23.0 require github.com/blugnu/test v0.12.0
There was a problem hiding this comment.
Actionable comments posted: 5
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (4)
state.go (1)
13-34: Update State docs to referenceRestore.After renaming the method, the State documentation and example still instruct callers to defer
Reset, which no longer exists. Please align the comment with the newRestorename.-// the test has run (by deferring a call to the Reset method on the +// the test has run (by deferring a call to the Restore method on the // returned state value). @@ -// defer env.State().Reset() +// defer env.State().Restore()env.go (1)
55-58: Route Lookup through internal for seam consistency.Align with IsSet/Set/Unset and improve testability by using internal.LookupEnv.
func Lookup(name string) (string, bool) { - return os.LookupEnv(name) + return internal.LookupEnv(name) }errors.go (2)
140-143: Fix doc: Unwrap refers to ParseError, not env.Parse.Update comment to match the type.
-// Unwrap returns the error that caused the env.Parse. +// Unwrap returns the error that caused the ParseError. func (e ParseError) Unwrap() error { return e.Err }
218-225: Fix doc: align RangeError.Error() comment with returned strings.Prepend env.RangeError in examples to match implementation.
-// out of range: <Min> <= (x) <= <Max> +// env.RangeError: <Min> <= (x) <= <Max> @@ -// out of range +// env.RangeError
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (37)
.golangci.yml(1 hunks)README.md(4 hunks)as/as_test.go(0 hunks)as/bool_test.go(1 hunks)as/duration.go(1 hunks)as/duration_test.go(1 hunks)as/filename_test.go(1 hunks)as/funcs.go(0 hunks)as/int.go(1 hunks)as/int_test.go(1 hunks)as/internal/funcs.go(1 hunks)as/pkgdoc.go(1 hunks)as/portNo.go(2 hunks)as/portNo_test.go(1 hunks)as/string_test.go(1 hunks)as/url.go(2 hunks)as/url_test.go(1 hunks)env.go(4 hunks)env_test.go(1 hunks)errors.go(4 hunks)errors_test.go(1 hunks)funcs.go(0 hunks)funcs_test.go(0 hunks)go.mod(1 hunks)internal/consts.go(1 hunks)internal/funcs.go(1 hunks)internal/funcs_test.go(1 hunks)internal/pkgdoc.go(1 hunks)load.go(1 hunks)load_test.go(1 hunks)parse.go(2 hunks)parse_test.go(1 hunks)pkgdoc.go(1 hunks)state.go(3 hunks)state_test.go(1 hunks)vars.go(1 hunks)vars_test.go(1 hunks)
💤 Files with no reviewable changes (4)
- funcs_test.go
- as/funcs.go
- funcs.go
- as/as_test.go
🧰 Additional context used
🧠 Learnings (14)
📚 Learning: 2024-07-04T09:04:46.594Z
Learnt from: deltics
PR: blugnu/env#2
File: load.go:94-121
Timestamp: 2024-07-04T09:04:46.594Z
Learning: When suggesting the use of `bufio.Scanner` for reading lines in files, consider the potential impact on testing complexity, especially if efficiency is not critical.
Applied to files:
load.goload_test.go
📚 Learning: 2025-09-21T02:35:04.766Z
Learnt from: deltics
PR: blugnu/env#10
File: load.go:163-171
Timestamp: 2025-09-21T02:35:04.766Z
Learning: The default bufio.Scanner uses ScanLines which automatically removes both \r and \n characters when reading lines, so manual CRLF handling is not needed when using scanner.Scan().
Applied to files:
load.go
📚 Learning: 2025-09-24T23:02:26.831Z
Learnt from: deltics
PR: blugnu/env#10
File: as/filename.go:21-29
Timestamp: 2025-09-24T23:02:26.831Z
Learning: In as/filename.go, differentiate between empty filename conditions and file-not-existing conditions using distinct sentinel errors for better semantic clarity.
Applied to files:
load.gointernal/funcs.goerrors_test.goas/filename_test.goerrors.go
📚 Learning: 2024-07-04T08:06:24.732Z
Learnt from: deltics
PR: blugnu/env#2
File: load_test.go:3-10
Timestamp: 2024-07-04T08:06:24.732Z
Learning: The `io/fs` package is used in the `load_test.go` file, specifically `fs.ErrNotExist`.
Applied to files:
load.gointernal/funcs_test.gointernal/funcs.goas/filename_test.go
📚 Learning: 2025-09-21T03:03:26.651Z
Learnt from: deltics
PR: blugnu/env#10
File: state.go:65-66
Timestamp: 2025-09-21T03:03:26.651Z
Learning: In the blugnu/env codebase, test seams (like internal package wrappers around OS functions) should only be introduced where required for testing, not for consistency alone. The practice is to use seams only where needed rather than universally wrapping all OS calls.
Applied to files:
vars.gointernal/funcs.gostate.go
📚 Learning: 2025-09-24T23:20:43.209Z
Learnt from: deltics
PR: blugnu/env#10
File: internal/func_test.go:16-18
Timestamp: 2025-09-24T23:20:43.209Z
Learning: In tests, prefer using actual values that consumers would use (like ".env") rather than internal constants, even when those constants exist. Tests should reflect real-world usage patterns, not implementation details.
Applied to files:
internal/funcs_test.go
📚 Learning: 2025-09-13T00:47:52.975Z
Learnt from: deltics
PR: blugnu/env#10
File: as/int.go:3-6
Timestamp: 2025-09-13T00:47:52.975Z
Learning: fmt.Errorf supports multiple %w verbs in a single format string. When multiple %w verbs are present, the returned error implements an Unwrap method returning a []error containing all the %w operands in the order they appear in the arguments. This allows wrapping multiple errors in a single fmt.Errorf call.
Applied to files:
as/int.goas/duration.goas/int_test.go
📚 Learning: 2025-09-24T23:04:07.744Z
Learnt from: deltics
PR: blugnu/env#10
File: as/duration_test.go:111-118
Timestamp: 2025-09-24T23:04:07.744Z
Learning: The ErrNotAnIntegerOrDuration error in the as package is implemented as a combinedError that allows testing for any constituent error (ErrNotAnInteger, ErrNotADuration, or strconv.ErrSyntax) using errors.Is(), providing flexible error handling for callers.
Applied to files:
as/int.goas/duration.goas/int_test.goas/duration_test.go
📚 Learning: 2024-07-04T08:27:07.225Z
Learnt from: deltics
PR: blugnu/env#2
File: load.go:63-91
Timestamp: 2024-07-04T08:27:07.225Z
Learning: When suggesting improvements for error handling, ensure to verify that all types of errors are adequately covered in the tests, including multiple errors being joined.
Applied to files:
as/portNo_test.goerrors_test.goas/duration_test.go
📚 Learning: 2025-09-15T03:59:10.905Z
Learnt from: deltics
PR: blugnu/env#10
File: internal/consts.go:3-6
Timestamp: 2025-09-15T03:59:10.905Z
Learning: In Go, filepath.Join cannot be used in const declarations since it's a function call. For simple relative path prefixes like "./", string concatenation is cross-platform compatible and preferable to using vars with filepath.Join.
Applied to files:
internal/consts.go
📚 Learning: 2025-09-24T22:24:17.657Z
Learnt from: deltics
PR: blugnu/env#10
File: .golangci.yml:1-3
Timestamp: 2025-09-24T22:24:17.657Z
Learning: In the blugnu/env project, deltics prefers to rely on golangci-lint's automatic Go version detection from go.mod rather than explicit run.go configuration, as it's more robust and avoids version synchronization issues. The project size doesn't warrant explicit timeout configuration.
Applied to files:
go.mod.golangci.yml
📚 Learning: 2025-09-21T03:06:50.819Z
Learnt from: deltics
PR: blugnu/env#10
File: state_test.go:25-42
Timestamp: 2025-09-21T03:06:50.819Z
Learning: When testing environment state management functions, don't use those same functions as safety mechanisms in tests. This creates circular dependencies where test infrastructure relies on the system under test, potentially causing cascading failures.
Applied to files:
state_test.go
📚 Learning: 2025-09-12T18:30:13.036Z
Learnt from: deltics
PR: blugnu/env#10
File: .coderabbit.yaml:4-8
Timestamp: 2025-09-12T18:30:13.036Z
Learning: CodeRabbit now supports golangci-lint v2, so separate v1 configuration files for CodeRabbit are no longer necessary. Both v1 and v2 versions are supported by CodeRabbit's tooling.
Applied to files:
.golangci.yml
📚 Learning: 2025-09-24T22:33:15.369Z
Learnt from: deltics
PR: blugnu/env#10
File: .golangci.yml:21-24
Timestamp: 2025-09-24T22:33:15.369Z
Learning: In the blugnu/env project, deltics prefers to keep the intrange linter disabled, favoring explicit loop constructs over int ranges for clarity in individual circumstances.
Applied to files:
.golangci.yml
🧬 Code graph analysis (24)
load.go (4)
state.go (1)
State(38-40)internal/consts.go (1)
EnvFile(5-5)internal/funcs.go (5)
FileExists(38-48)NewFileReader(61-63)ScanLines(19-19)LookupEnv(16-16)Setenv(22-22)errors.go (3)
NewFileError(33-38)ErrInvalidEntry(11-11)ErrSetFailed(19-19)
vars.go (1)
internal/funcs.go (1)
LookupEnv(16-16)
internal/funcs_test.go (1)
internal/funcs.go (4)
FileExists(38-48)Stat(25-25)Open(13-13)NewFileReader(61-63)
as/int.go (1)
as/errors.go (1)
ErrNotAnInteger(29-29)
as/portNo_test.go (2)
as/portNo.go (1)
PortNo(32-47)errors.go (1)
RangeError(213-216)
state.go (1)
internal/funcs.go (1)
Setenv(22-22)
load_test.go (5)
internal/funcs.go (5)
FileExists(38-48)NewFileReader(61-63)Setenv(22-22)LookupEnv(16-16)ScanLines(19-19)state.go (1)
State(38-40)load.go (3)
Load(33-74)LoadFile(86-102)LoadFromReader(112-158)internal/consts.go (1)
EnvFile(5-5)errors.go (2)
FileError(28-31)ErrInvalidEntry(11-11)
env.go (1)
internal/funcs.go (3)
LookupEnv(16-16)Setenv(22-22)Unsetenv(28-28)
errors_test.go (1)
errors.go (5)
FileError(28-31)NewFileError(33-38)ParseError(84-87)InvalidValueError(148-151)RangeError(213-216)
as/portNo.go (1)
errors.go (1)
RangeError(213-216)
parse_test.go (3)
internal/funcs.go (1)
LookupEnv(16-16)parse.go (2)
Parse(55-73)ParseInto(97-109)errors.go (4)
ErrNotSet(15-15)ParseError(84-87)InvalidValueError(148-151)ErrTargetIsNil(22-22)
state_test.go (2)
state.go (1)
State(38-40)internal/funcs.go (1)
Setenv(22-22)
as/bool_test.go (1)
as/bool.go (1)
Bool(15-28)
as/duration.go (1)
as/errors.go (1)
ErrNotADuration(30-30)
as/filename_test.go (2)
as/filename.go (1)
Filename(9-28)as/errors.go (2)
ErrFilenameIsEmpty(27-27)ErrFilenameIsDirectory(28-28)
as/string_test.go (1)
as/string.go (1)
String(14-16)
as/url.go (1)
as/internal/funcs.go (1)
ParseURL(10-10)
vars_test.go (3)
state.go (1)
State(38-40)internal/funcs.go (1)
Setenv(22-22)vars.go (1)
Vars(29-60)
as/int_test.go (2)
as/int.go (1)
Int(19-25)as/errors.go (1)
ErrNotAnInteger(29-29)
errors.go (1)
as/filename.go (1)
Filename(9-28)
as/duration_test.go (2)
as/duration.go (2)
Duration(14-21)DurationIn(32-50)as/errors.go (3)
ErrNotADuration(30-30)ErrNotAnInteger(29-29)ErrNotAnIntegerOrDuration(31-31)
as/url_test.go (2)
as/url.go (1)
AbsoluteURL(31-40)as/errors.go (1)
ErrNotAnAbsoluteURL(32-32)
parse.go (2)
errors.go (4)
ParseError(84-87)InvalidValueError(148-151)ErrNotSet(15-15)ErrTargetIsNil(22-22)internal/funcs.go (1)
LookupEnv(16-16)
env_test.go (2)
internal/funcs.go (2)
Setenv(22-22)Unsetenv(28-28)env.go (6)
Clear(11-13)IsSet(35-38)Get(28-30)Lookup(56-58)Set(74-76)Unset(91-98)
🪛 GitHub Actions: release
go.mod
[error] 1-1: CI script error: file does not exist (./go.mod).
🔇 Additional comments (36)
README.md (1)
55-66: Clearer default-handling guidanceThe rewritten example and narrative make it obvious how
env.Parsebehaves when the variable is missing vs. set, which will save readers a double-take. Nice improvement to the docs.internal/pkgdoc.go (1)
1-11: LGTM – clear internal package doc with SPDX header.internal/consts.go (1)
3-6: LGTM – simple exported constant with proper doc comment.as/bool_test.go (1)
13-67: LGTM – cases align with implementation, including whitespace and mixed‑case yes/no.go.mod (2)
3-3: Pin the Go toolchain for consistent local/CI builds.Add a toolchain directive to avoid version drift between dev and CI.
module github.com/blugnu/env go 1.23 +toolchain go1.23.0
3-5: Release workflow likely running outside repo root; fix CI to resolve “file does not exist (./go.mod)”.Ensure the release job:
- runs actions/checkout first,
- uses actions/setup-go with go-version-file: go.mod,
- executes from the repository root (set working-directory: . or remove custom cwd).
as/string_test.go (1)
11-22: Nice, focused coverage foras.String.The test nails the happy-path contract and uses the shared harness cleanly. No further gaps spotted here.
as/internal/funcs.go (1)
1-11: Doc-comment fix keeps the seam tidy.The alias (and its docstring) now line up with godoclint while preserving the override seam—looks good.
pkgdoc.go (1)
1-22: Package doc reads well.Great high-level overview of env’s responsibilities with the SPDX tag in place.
.golangci.yml (1)
1-51: Lint config dialed in.Defaulting to
all, carving out only the project-specific disables, and keepingstaticcheckactive with a targeted exclusion is a solid balance.vars_test.go (1)
12-66: Vars scenarios are well covered.The harness isolation plus the new whitespace/value cases map cleanly onto the API contract.
internal/funcs_test.go (1)
22-107: Thorough exercise of the IO seams.Thanks for sticking with consumer-style inputs (per your earlier guidance)—the Stat/Open branches and close-on-success paths are all verified.
internal/funcs.go (1)
10-63: IO seam wrappers andFileExistsbehaviour look right.Explicitly handling
ErrNotExistwhile still honouring returnedFileInfos matches the documented nuance;NewFileReaderstays a simple passthrough seam.as/duration.go (1)
4-48: Duration parsing refactor looks solid.Whitespace trimming plus the fallback/dual-wrapped error path cover the intended UX without regressing the sentinels.
as/duration_test.go (1)
123-131: Great validation of the combined sentinel behaviour.
Thanks for locking in the expectation that the decimal-path error unwraps toErrNotADuration,ErrNotAnInteger, andErrNotAnIntegerOrDuration(per the combined-error design we discussed earlier). This keeps callers free to probe the flavour they care about.state_test.go (1)
32-39: Appreciate the OS-level safety net.
Capturing and restoring the env with plainos/stringscalls avoids the circular dependency on the code under test—exactly the isolation we talked about before. Nicely done.load.go (1)
41-57: Path cleaning + de-dupe hits the mark.
filepath.Cleanfollowed by theseenfilter eliminates./.envvs.envdouble loads while still honouring the “.env first” rule. Clean, predictable ordering.vars.go (1)
35-56: Trimmed keys keep the contract crisp.
Switching both pathways to emit already-trimmed keys means the returned map matches the documented semantics with no downstream surprises.as/int.go (1)
22-23: Nice double-wrap to preserve context.
The dual%wkeeps bothErrNotAnIntegerand the underlyingstrconverror reachable viaerrors.Is, so callers can branch however they like without losing detail.as/url.go (1)
32-35: Internal ParseURL seam fits nicely.
Delegating tointernal.ParseURLkeeps the parsing stub-able while still returning the rawurl.Parseerror (with the wrapcheck note), which is exactly what downstream code expects.as/portNo_test.go (1)
23-28: Solid bounds assertion.
Verifying theRangeErrorviaerrors.Asand checking the recorded min/max nails the public contract for out-of-range ports.as/int_test.go (1)
37-40: Good dual-sentinel check.
Asserting bothstrconv.ErrSyntaxandErrNotAnIntegerensures the unwrap chain stays intact for callers matching on either error.env_test.go (2)
14-26: Good migration to public API and per-test restoration.Tests now use env.IsSet/Get/Clear and env.State().Restore for isolation. Looks solid.
100-149: Nice error-path seam via internal.Unsetenv.The stub and errors.Is assertions correctly verify wrapping and rollback behavior.
env.go (1)
92-98: LGTM: error wrapping and Windows semantics documented.env.Unset wraps errors with context and preserves atomic semantics per key.
load_test.go (3)
100-111: Great precedence test for .env first.This ensures .env wins regardless of argument order. Nice.
219-239: Add comment-with-leading-space coverage.Confirms trimmed comments are ignored.
@@ Case("contains empty lines", testcase{ @@ }), + + Case("comment lines with leading spaces are ignored", testcase{ + content: " # c1\n\t # c2\nVAR=ok\n", + assert: func(err error) { + Expect(err).IsNil() + Expect(os.Getenv("VAR")).To(Equal("ok")) + }, + }),
66-77: Add case: pre-set empty vars aren’t overwritten.Verifies “do not replace existing variables (even if empty)” behavior.
@@ Case("no files specified/.env exists with valid entries", testcase{ @@ }), + + Case("pre-set empty value is not overwritten", testcase{ + fileContent: map[string]any{ + internal.EnvFile: "X=from-file\nY=from-file", + }, + assert: func(err error) { + Expect(err).IsNil() + Expect(os.Getenv("X")).To(Equal("from-proc")) + Expect(os.Getenv("Y")).To(Equal("")) + }, + }),Seed X/Y in the arrange section:
@@ defer env.State().Restore() os.Clearenv() - t.Setenv("PRESET", "true") + t.Setenv("PRESET", "true") + _ = os.Setenv("X", "from-proc") + _ = os.Setenv("Y", "")parse.go (2)
55-73: Parse: clear semantics and correct wrapping.Correct use of internal.LookupEnv, default handling, and ParseError wrapping.
97-109: ParseInto: nil-target guard and error shape are correct.Returns ParseError{Err: ErrTargetIsNil} and only assigns on success. Good.
errors.go (1)
40-52: FileError message and Is/Unwrap implementations look good.Consistent env.* prefixes and robust errors.Is support for pointer/value targets.
as/portNo.go (1)
33-43: Consistent error context and trimming look solid.Thank you for hardening the parser with TrimSpace and the uniform
as.PortNoerror prefix; this keeps callers’ diagnostics aligned with the rest of the package.as/filename_test.go (1)
37-52: Great cross-platform coverage.The split between non-existent, invalid-byte, directory, and empty-path cases gives us confidence the new API behaves correctly on every OS.
parse_test.go (1)
98-149: Thorough default/error-path assertions appreciated.Exercising both
ParseandParseIntofor success, default fallback, and wrapped error propagation will catch regressions quickly.as/url_test.go (1)
25-48: Field-wise URL assertions keep the test stable.Validating
SchemeandHostis a nice improvement over struct equality and should survive stdlib changes.errors_test.go (1)
333-414: Comprehensive pointer/value Is() matrix looks great.The added pointer-target cases make the
InvalidValueErrorbehaviour explicit and symmetric with the other error types.
There was a problem hiding this comment.
Actionable comments posted: 7
Caution
Some comments are outside the diff and can’t be posted inline due to platform limitations.
⚠️ Outside diff range comments (1)
state.go (1)
42-66: Update theStatedoc block to match theRestorerename.The top-of-file comment still mentions calling
Reset; that’s now stale after the rename and will trip readers (and doc tooling). Please switch both the narrative sentence and the example toRestore.-// the test has run (by deferring a call to the Reset method on the +// the test has run (by deferring a call to the Restore method on the // returned state value). @@ -// defer env.State().Reset() +// defer env.State().Restore()
📜 Review details
Configuration used: Path: .coderabbit.yaml
Review profile: ASSERTIVE
Plan: Pro
⛔ Files ignored due to path filters (1)
go.sumis excluded by!**/*.sum
📒 Files selected for processing (37)
.golangci.yml(1 hunks)README.md(4 hunks)as/as_test.go(0 hunks)as/bool_test.go(1 hunks)as/duration.go(1 hunks)as/duration_test.go(1 hunks)as/filename_test.go(1 hunks)as/funcs.go(0 hunks)as/int.go(1 hunks)as/int_test.go(1 hunks)as/internal/funcs.go(1 hunks)as/pkgdoc.go(1 hunks)as/portNo.go(2 hunks)as/portNo_test.go(1 hunks)as/string_test.go(1 hunks)as/url.go(2 hunks)as/url_test.go(1 hunks)env.go(4 hunks)env_test.go(1 hunks)errors.go(4 hunks)errors_test.go(1 hunks)funcs.go(0 hunks)funcs_test.go(0 hunks)go.mod(1 hunks)internal/consts.go(1 hunks)internal/funcs.go(1 hunks)internal/funcs_test.go(1 hunks)internal/pkgdoc.go(1 hunks)load.go(1 hunks)load_test.go(1 hunks)parse.go(2 hunks)parse_test.go(1 hunks)pkgdoc.go(1 hunks)state.go(3 hunks)state_test.go(1 hunks)vars.go(1 hunks)vars_test.go(1 hunks)
💤 Files with no reviewable changes (4)
- as/funcs.go
- funcs_test.go
- as/as_test.go
- funcs.go
🧰 Additional context used
🧠 Learnings (15)
📚 Learning: 2025-09-13T00:47:52.975Z
Learnt from: deltics
PR: blugnu/env#10
File: as/int.go:3-6
Timestamp: 2025-09-13T00:47:52.975Z
Learning: fmt.Errorf supports multiple %w verbs in a single format string. When multiple %w verbs are present, the returned error implements an Unwrap method returning a []error containing all the %w operands in the order they appear in the arguments. This allows wrapping multiple errors in a single fmt.Errorf call.
Applied to files:
as/int.goas/duration.goas/int_test.go
📚 Learning: 2025-09-24T23:04:07.744Z
Learnt from: deltics
PR: blugnu/env#10
File: as/duration_test.go:111-118
Timestamp: 2025-09-24T23:04:07.744Z
Learning: The ErrNotAnIntegerOrDuration error in the as package is implemented as a combinedError that allows testing for any constituent error (ErrNotAnInteger, ErrNotADuration, or strconv.ErrSyntax) using errors.Is(), providing flexible error handling for callers.
Applied to files:
as/int.goas/duration.goas/int_test.goas/duration_test.go
📚 Learning: 2025-09-12T18:30:13.036Z
Learnt from: deltics
PR: blugnu/env#10
File: .coderabbit.yaml:4-8
Timestamp: 2025-09-12T18:30:13.036Z
Learning: CodeRabbit now supports golangci-lint v2, so separate v1 configuration files for CodeRabbit are no longer necessary. Both v1 and v2 versions are supported by CodeRabbit's tooling.
Applied to files:
.golangci.yml
📚 Learning: 2025-09-24T22:24:17.657Z
Learnt from: deltics
PR: blugnu/env#10
File: .golangci.yml:1-3
Timestamp: 2025-09-24T22:24:17.657Z
Learning: In the blugnu/env project, deltics prefers to rely on golangci-lint's automatic Go version detection from go.mod rather than explicit run.go configuration, as it's more robust and avoids version synchronization issues. The project size doesn't warrant explicit timeout configuration.
Applied to files:
.golangci.ymlgo.mod
📚 Learning: 2025-09-24T22:33:15.369Z
Learnt from: deltics
PR: blugnu/env#10
File: .golangci.yml:21-24
Timestamp: 2025-09-24T22:33:15.369Z
Learning: In the blugnu/env project, deltics prefers to keep the intrange linter disabled, favoring explicit loop constructs over int ranges for clarity in individual circumstances.
Applied to files:
.golangci.yml
📚 Learning: 2024-07-04T08:06:24.732Z
Learnt from: deltics
PR: blugnu/env#2
File: load_test.go:3-10
Timestamp: 2024-07-04T08:06:24.732Z
Learning: The `io/fs` package is used in the `load_test.go` file, specifically `fs.ErrNotExist`.
Applied to files:
internal/funcs_test.gointernal/funcs.goload.goas/filename_test.go
📚 Learning: 2025-09-24T23:20:43.209Z
Learnt from: deltics
PR: blugnu/env#10
File: internal/func_test.go:16-18
Timestamp: 2025-09-24T23:20:43.209Z
Learning: In tests, prefer using actual values that consumers would use (like ".env") rather than internal constants, even when those constants exist. Tests should reflect real-world usage patterns, not implementation details.
Applied to files:
internal/funcs_test.go
📚 Learning: 2024-07-04T08:27:07.225Z
Learnt from: deltics
PR: blugnu/env#2
File: load.go:63-91
Timestamp: 2024-07-04T08:27:07.225Z
Learning: When suggesting improvements for error handling, ensure to verify that all types of errors are adequately covered in the tests, including multiple errors being joined.
Applied to files:
errors_test.goas/duration_test.goas/portNo_test.go
📚 Learning: 2025-09-24T23:02:26.831Z
Learnt from: deltics
PR: blugnu/env#10
File: as/filename.go:21-29
Timestamp: 2025-09-24T23:02:26.831Z
Learning: In as/filename.go, differentiate between empty filename conditions and file-not-existing conditions using distinct sentinel errors for better semantic clarity.
Applied to files:
errors_test.gointernal/funcs.goerrors.goload.goas/filename_test.go
📚 Learning: 2025-09-21T03:03:26.651Z
Learnt from: deltics
PR: blugnu/env#10
File: state.go:65-66
Timestamp: 2025-09-21T03:03:26.651Z
Learning: In the blugnu/env codebase, test seams (like internal package wrappers around OS functions) should only be introduced where required for testing, not for consistency alone. The practice is to use seams only where needed rather than universally wrapping all OS calls.
Applied to files:
internal/funcs.gostate.govars.go
📚 Learning: 2025-09-21T03:06:50.819Z
Learnt from: deltics
PR: blugnu/env#10
File: state_test.go:25-42
Timestamp: 2025-09-21T03:06:50.819Z
Learning: When testing environment state management functions, don't use those same functions as safety mechanisms in tests. This creates circular dependencies where test infrastructure relies on the system under test, potentially causing cascading failures.
Applied to files:
state_test.go
📚 Learning: 2025-09-15T03:59:10.905Z
Learnt from: deltics
PR: blugnu/env#10
File: internal/consts.go:3-6
Timestamp: 2025-09-15T03:59:10.905Z
Learning: In Go, filepath.Join cannot be used in const declarations since it's a function call. For simple relative path prefixes like "./", string concatenation is cross-platform compatible and preferable to using vars with filepath.Join.
Applied to files:
internal/consts.go
📚 Learning: 2025-09-25T03:13:16.923Z
Learnt from: deltics
PR: blugnu/env#10
File: as/pkgdoc.go:1-6
Timestamp: 2025-09-25T03:13:16.923Z
Learning: In Go doc comments, when referencing packages within the same module, use the short form [pkg.Symbol] rather than fully qualified links [module.com/pkg.Symbol]. The short form is the preferred and idiomatic approach for intra-module references in Go documentation, while fully qualified paths are typically reserved for external packages or cases where ambiguity might exist.
Applied to files:
as/pkgdoc.go
📚 Learning: 2024-07-04T09:04:46.594Z
Learnt from: deltics
PR: blugnu/env#2
File: load.go:94-121
Timestamp: 2024-07-04T09:04:46.594Z
Learning: When suggesting the use of `bufio.Scanner` for reading lines in files, consider the potential impact on testing complexity, especially if efficiency is not critical.
Applied to files:
load.goload_test.go
📚 Learning: 2025-09-21T02:35:04.766Z
Learnt from: deltics
PR: blugnu/env#10
File: load.go:163-171
Timestamp: 2025-09-21T02:35:04.766Z
Learning: The default bufio.Scanner uses ScanLines which automatically removes both \r and \n characters when reading lines, so manual CRLF handling is not needed when using scanner.Scan().
Applied to files:
load.go
🧬 Code graph analysis (24)
as/portNo.go (1)
errors.go (1)
RangeError(213-216)
as/int.go (1)
as/errors.go (1)
ErrNotAnInteger(29-29)
as/duration.go (1)
as/errors.go (1)
ErrNotADuration(30-30)
as/url_test.go (2)
as/url.go (1)
AbsoluteURL(31-40)as/errors.go (1)
ErrNotAnAbsoluteURL(32-32)
internal/funcs_test.go (1)
internal/funcs.go (4)
FileExists(38-48)Stat(25-25)Open(13-13)NewFileReader(61-63)
env_test.go (3)
state.go (1)
State(38-40)internal/funcs.go (2)
Setenv(22-22)Unsetenv(28-28)env.go (6)
Clear(11-13)IsSet(35-38)Get(28-30)Lookup(56-58)Set(74-76)Unset(91-98)
errors_test.go (1)
errors.go (5)
FileError(28-31)NewFileError(33-38)ParseError(84-87)InvalidValueError(148-151)RangeError(213-216)
state_test.go (1)
state.go (1)
State(38-40)
as/int_test.go (2)
as/int.go (1)
Int(19-25)as/errors.go (1)
ErrNotAnInteger(29-29)
vars_test.go (3)
state.go (1)
State(38-40)internal/funcs.go (1)
Setenv(22-22)vars.go (1)
Vars(29-60)
errors.go (1)
as/filename.go (1)
Filename(9-28)
env.go (1)
internal/funcs.go (3)
LookupEnv(16-16)Setenv(22-22)Unsetenv(28-28)
load.go (4)
state.go (1)
State(38-40)internal/consts.go (1)
EnvFile(5-5)internal/funcs.go (5)
FileExists(38-48)NewFileReader(61-63)ScanLines(19-19)LookupEnv(16-16)Setenv(22-22)errors.go (3)
NewFileError(33-38)ErrInvalidEntry(11-11)ErrSetFailed(19-19)
as/bool_test.go (1)
as/bool.go (1)
Bool(15-28)
as/string_test.go (1)
as/string.go (1)
String(14-16)
as/duration_test.go (2)
as/duration.go (2)
Duration(14-21)DurationIn(32-50)as/errors.go (3)
ErrNotADuration(30-30)ErrNotAnInteger(29-29)ErrNotAnIntegerOrDuration(31-31)
as/filename_test.go (2)
as/filename.go (1)
Filename(9-28)as/errors.go (2)
ErrFilenameIsEmpty(27-27)ErrFilenameIsDirectory(28-28)
state.go (1)
internal/funcs.go (1)
Setenv(22-22)
load_test.go (5)
internal/funcs.go (5)
FileExists(38-48)NewFileReader(61-63)Setenv(22-22)LookupEnv(16-16)ScanLines(19-19)state.go (1)
State(38-40)load.go (3)
Load(33-74)LoadFile(86-102)LoadFromReader(112-158)internal/consts.go (1)
EnvFile(5-5)errors.go (2)
FileError(28-31)ErrInvalidEntry(11-11)
as/url.go (1)
as/internal/funcs.go (1)
ParseURL(10-10)
parse.go (2)
errors.go (4)
ParseError(84-87)InvalidValueError(148-151)ErrNotSet(15-15)ErrTargetIsNil(22-22)internal/funcs.go (1)
LookupEnv(16-16)
vars.go (1)
internal/funcs.go (1)
LookupEnv(16-16)
parse_test.go (3)
internal/funcs.go (1)
LookupEnv(16-16)parse.go (2)
Parse(55-73)ParseInto(97-109)errors.go (4)
ErrNotSet(15-15)ParseError(84-87)InvalidValueError(148-151)ErrTargetIsNil(22-22)
as/portNo_test.go (2)
as/portNo.go (1)
PortNo(32-47)errors.go (1)
RangeError(213-216)
🪛 GitHub Actions: release
go.mod
[error] 1-1: go mod tidy is required; updated go.mod/go.sum must be committed (command: go mod tidy)
🔇 Additional comments (36)
.golangci.yml (1)
48-51: Nice catch keeping staticcheck fully enabled.Adding
allback in before subtracting-QF1008restores the complete staticcheck suite while still silencing the noisy advisory. Looks good.as/pkgdoc.go (1)
1-9: Doc comment stays consistent with Go intra-module links.
Retaining the short-form [env.Parse]/[env.ParseInto] references (per the Go doc guidance we discussed earlier) keeps pkgsite happy and matches module conventions.as/bool_test.go (1)
13-67: Matrix now mirrors Bool converter semantics.
The whitespace variants and empty-token coverage keep these assertions aligned with as.Bool’s trim-and-lowercase behavior.pkgdoc.go (1)
1-22: Package doc clearly frames the expanded API.
Good overview of parsing, loading, and test-support capabilities, plus the SPDX header.as/string_test.go (1)
11-23: Straightforward happy-path coverage looks good.
Confirms the converter is an identity function and will catch accidental changes.vars.go (1)
10-59: Implementation matches the documented trimming semantics.
Both code paths trim keys exactly once and the map preallocation is tidy.as/int.go (1)
20-23: Error wrapping keeps the sentinel and root cause intact.
The dual%wpropagates ErrNotAnInteger alongside the original strconv failure just as intended.internal/consts.go (1)
3-6: Doc comment succinctly documents the constant.
Enough context for pkgdoc and lint tooling without extra noise.internal/pkgdoc.go (1)
1-11: Internal package doc is concise and SPDX-complete.
Nicely signals that the internals are module-private and subject to change.as/int_test.go (1)
22-41: Nice coverage of sentinel unwrapping.Thanks for asserting both
strconv.ErrSyntaxandas.ErrNotAnInteger; this locks down the multi-%wcontract while keeping the happy path boundaries in check.as/internal/funcs.go (1)
8-10: Docstring tweak keeps godoclint satisfied.Appreciate the rename plus comment fix—this keeps the alias lint-clean while still advertising the test seam.
as/portNo_test.go (1)
23-54: Solid boundary assertions on RangeError.Really like the
errors.Asprobe for Min/Max; that guarantees the wrappedRangeError[int]stays consistent if we ever rejig the formatter.as/url.go (1)
32-34: Directly surfacing the stdlib parse error is the right call.Keeping the original
*url.Errorintact avoids double-wrapping noise and plays nicely with callers already matching onurl.Parsefailures.as/portNo.go (1)
33-46: Whitespace trim + shared handler reads well.The
TrimSpaceplushandleErrorhelper keeps the branches tidy while guaranteeing every exit path carries theas.PortNo:prefix. Nicely done.as/filename_test.go (1)
29-68: Good cross-platform guards on the invalid path case.Checking for “non-nil and not
os.ErrNotExist” makes this resilient to the syscall differences we see across CI runners.as/url_test.go (1)
25-47: Field-level assertions keep this future-proof.Validating scheme/host instead of the whole struct dodges surprises if
url.URLgrows new fields. Thanks for tightening that up.parse_test.go (2)
59-63: Good structured error assertions.Asserting ParseError + InvalidValueError (and the underlying cause) makes failure modes explicit and stable.
17-21: Deduplicate LookupEnv stubbing with a tiny helper.Define a withLookupEnv(val string, ok bool) helper returning a restore func, then replace repeated inline stubs. Keeps tests terse and centralizes restore logic.
@@ import ( "errors" "strconv" "testing" . "github.com/blugnu/test" "github.com/blugnu/env" "github.com/blugnu/env/internal" ) +// withLookupEnv replaces internal.LookupEnv for a test and returns a restore func. +func withLookupEnv(val string, ok bool) func() { + return Restore(Original(&internal.LookupEnv).ReplacedBy( + func(string) (string, bool) { return val, ok }, + )) +} @@ - defer Restore(Original(&internal.LookupEnv).ReplacedBy(func(string) (string, bool) { - return "123", true - })) + defer withLookupEnv("123", true)()Apply similarly to the other repeated stubs in this file.
Also applies to: 33-36, 52-55, 70-73, 86-89, 102-105, 121-124, 138-141
env_test.go (1)
91-98: LGTM: public API usage and empty-string presence case covered.Solid isolation via State().Restore and Setenv; Lookup behavior for empty values is locked in.
internal/funcs_test.go (1)
61-69: LGTM: directory, non-nil FileInfo+error, and ENOENT branches are exercised.Good breadth on FileExists behavior; NewFileReader seams are also well tested.
errors_test.go (1)
235-263: LGTM: pointer-target Is cases included.Comprehensive coverage for ParseError (and likewise for others elsewhere in the file) ensures consistent semantics across value and pointer targets.
vars_test.go (1)
47-65: LGTM: key trimming and value preservation verified.This nails the intended Vars contract for both specified names and value whitespace.
env.go (1)
32-38: LGTM: test seams and error context.
- IsSet uses internal.LookupEnv for stubbability.
- Set/Unset route via internal seams; Unset error prefix is consistent (“env.Unset: …”).
Also applies to: 71-76, 92-95
internal/funcs.go (1)
10-29: LGTM: seams documented and FileExists behavior clarified.
- Godoclint-friendly comments and gochecknoglobals rationale look good.
- FileExists explicitly handles ErrNotExist and directories while tolerating non-nil FileInfo.
Also applies to: 31-48
load_test.go (4)
100-110: Great coverage of .env precedence over explicit filesThis verifies ".env" is loaded first and non-overwrite semantics hold. Solid.
138-158: Nice dedup across path variantsThe filepath.Clean-based dedup ensures ".env", "./.env", etc., are loaded once. Good test.
255-275: Add case: leading-space comment lines are ignoredCovers " # comment" handling.
@@ Case("contains empty lines", testcase{ @@ }), + + Case("comment lines (with and without leading spaces) are ignored", testcase{ + content: "# c1\n # c2\nVAR=ok\n", + assert: func(err error) { + Expect(err).IsNil() + Expect(os.Getenv("VAR")).To(Equal("ok")) + }, + }),
112-121: Add case: do not overwrite an existing empty valueWe also preserve empty-string values; add a case to lock this in.
@@ Case("does not override existing environment variables", testcase{ @@ }), + + Case("does not override existing empty values", testcase{ + fileContent: map[string]any{ + internal.EnvFile: "X=from-file\nY=from-file", + }, + assert: func(err error) { + Expect(err).IsNil() + _ = os.Setenv("X", "") + _ = os.Setenv("Y", "") + Expect(os.Getenv("X")).To(Equal("")) + Expect(os.Getenv("Y")).To(Equal("")) + }, + }),state_test.go (1)
26-40: Good safety net: cleanup uses only os APIsAvoids circular dependency on the SUT. Nicely done.
as/duration.go (1)
45-46: Confirm errors.Is behaviour for notAnIntegerOrDurationErrorEnsure callers can do errors.Is(err, ErrNotAnInteger) and errors.Is(err, ErrNotADuration) against the returned error.
Please confirm that notAnIntegerOrDurationError implements errors.Is semantics (e.g., via multi-%w or Unwrap) so tests like errors.Is(err, ErrNotAnIntegerOrDuration), errors.Is(err, ErrNotAnInteger), and errors.Is(err, ErrNotADuration) all work as intended.
parse.go (2)
55-73: Parse error semantics and default handling look correct
- Wraps InvalidValueError when set-but-invalid.
- Wraps ErrNotSet when absent and no default.
- Honors empty-string as “set” (no default). Good.
97-109: Nil target guard in ParseInto is appropriateReturning ParseError with ErrTargetIsNil avoids panics and is consistent with docs.
load.go (3)
39-57: Path cleaning and dedup before existence checks is spot onCorrectly normalizes, dedups, and ensures .env precedence. Good defensive shape.
135-149: Validation and non-overwrite semantics handled correctly
- Empty/whitespace/comment lines skipped.
- Empty variable names flagged as ErrInvalidEntry.
- Pre-existing vars preserved.
- Set failures wrapped as ErrSetFailed.
151-157: Scanner error propagation keeps transactional guaranteeAppending scanner.Err() and joining ensures atomic rollback on reader/scanner failures. Good.
README.md (1)
16-17: No changes needed: badge alt text matches go.mod minimum version (1.23).
Summary by CodeRabbit
New Features
Refactor
Documentation
Chores
Tests