refactor(providers): export HasResolvedProviderValue and drop duplicate copies#329
Conversation
…te copies
The unresolved-${VAR} placeholder check was defined three times byte-for-byte:
in internal/providers/config.go (used by buildProviderConfig validation), in
internal/providers/vertex/vertex.go (used in validateConfig), and in
internal/providers/gemini/gemini.go (same shape). The function isn't
GCP-specific — it just looks for a literal `${` substring that would indicate
the YAML env-substitution pass left a placeholder unresolved.
Export the parent-package copy as `HasResolvedProviderValue`, delete the two
duplicates in vertex/ and gemini/, and rewrite the 6 child-package call sites
to use `providers.HasResolvedProviderValue`. Both child packages already
import `gomodel/internal/providers`, so no new dependencies.
Added a doc comment to the now-exported function explaining the
${VAR}-placeholder semantics so future readers don't have to infer them from
the implementation.
Test plan:
- Full `go test ./...` clean.
- `go test -race ./internal/providers/...` clean across providers,
providers/vertex, providers/gemini.
- Pre-commit hooks (test-race, lint, mod-tidy) pass.
Out of scope: the function is named `HasResolvedProviderValue` for continuity
with the existing usage; if a future caller outside provider config also wants
this check, renaming to something more generic (`HasResolvedConfigValue`?) is
a separate cosmetic change.
Co-Authored-By: Claude Opus 4.7 (1M context) <noreply@anthropic.com>
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Pro Run ID: 📒 Files selected for processing (3)
📝 WalkthroughWalkthroughThree provider implementations consolidate credential-resolution validation by exporting a shared helper from config.go and replacing duplicate local validators in Gemini and Vertex providers with calls to the new ChangesResolved-Value Validation Consolidation
Estimated code review effort🎯 2 (Simple) | ⏱️ ~10 minutes
🚥 Pre-merge checks | ✅ 4 | ❌ 1❌ Failed checks (1 warning)
✅ Passed checks (4 passed)
✏️ Tip: You can configure your own custom pre-merge checks in the settings. ✨ Finishing Touches📝 Generate docstrings
🧪 Generate unit tests (beta)
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 |
Greptile SummaryThis PR consolidates three byte-identical copies of a
Confidence Score: 5/5Safe to merge — pure deduplication with no behavior change and no new dependencies introduced. All three deleted functions were byte-identical to the kept one. The six rewritten call sites are mechanical one-token substitutions. Both child packages already imported the parent No files require special attention. Important Files Changed
Flowchart%%{init: {'theme': 'neutral'}}%%
flowchart TD
subgraph Before
A1["config.go\nhasResolvedProviderValue()"]
A2["gemini/gemini.go\nhasResolvedProviderValue()"]
A3["vertex/vertex.go\nhasResolvedProviderValue()"]
end
subgraph After
B["providers.HasResolvedProviderValue()\n(exported, documented)"]
C1["config.go\n5 call sites"]
C2["gemini/gemini.go\n2 call sites"]
C3["vertex/vertex.go\n2 call sites"]
C1 --> B
C2 --> B
C3 --> B
end
Reviews (1): Last reviewed commit: "refactor(providers): export HasResolvedP..." | Re-trigger Greptile |
|
Codecov Report❌ Patch coverage is
📢 Thoughts on this report? Let us know! |
2 participants
Summary
The unresolved-
${VAR}placeholder check was defined three times byte-for-byte (MD5-verified):internal/providers/config.go— used inbuildProviderConfigvalidationinternal/providers/vertex/vertex.go— used invalidateConfiginternal/providers/gemini/gemini.go— same shapeThe function isn't GCP-specific. It just looks for a literal
${substring that would indicate the YAML env-substitution pass left a placeholder unresolved (e.g.${OPENAI_API_KEY}when the env var wasn't set).This PR exports the parent-package copy as
HasResolvedProviderValue, deletes the two duplicates invertex/andgemini/, and rewrites the 6 child-package call sites to useproviders.HasResolvedProviderValue. Both child packages already importgomodel/internal/providers, so no new dependencies.Added a doc comment to the now-exported function explaining the
${VAR}-placeholder semantics so future readers don't have to infer them from the implementation.Risk
HasResolvedProviderValueis now public on theproviderspackage. Anyone outside this repo who needs the same check could call it. No downside — the function is small, well-defined, and the semantics are stable.vertex.goandgemini.goare 1 token longer (providers.HasResolvedProviderValuevshasResolvedProviderValue). Trivial.Out of scope
The name
HasResolvedProviderValuecarries the "Provider" qualifier purely for continuity with the existing usage. If a future caller outside provider config wants this check, a follow-up cosmetic rename (HasResolvedConfigValueor similar) is fine — not chasing it here.Test plan
go test ./...clean across the module.go test -raceclean on./internal/providers/....make test-race,make lint,go mod tidyall green.Net diff: -4 lines (16 inserted, including the new 6-line doc comment; 20 removed across 3 files).
🤖 Generated with Claude Code
Summary by CodeRabbit