refactor: introduce Client type, remove TUI from library, eliminate init() side effects#361
refactor: introduce Client type, remove TUI from library, eliminate init() side effects#361
Conversation
cde3419 to
fcb2c19
Compare
amoeba
left a comment
There was a problem hiding this comment.
Since this is too big to review in a reasonable amount of time, I just thought about the downstream use cases a bit.
This seems like the right direction to start moving but there'd still be quite a bit of work to make dbc loadable as a C extension. Which I assume is part of the purpose here. Stuff like exports, FFI-compatible types, etc.
The new Client interface with the Option pattern for customizing seems pretty reasonable. I'm not too familiar with Go but if we think the pattern is the most familiar than that works for me. One thing I found was my LSP wasn't super helpful with this pattern,
Here, it lists three of the candidates but not all,
I had to assume all of them were prefixed with With:
Any idea if there's a way to improve on this or if a different pattern works better with the Go LS?
For now, the purpose here is solely to make dbc useful to be imported by other Go programs instead of having to call out to it by spawning another process. I wasn't planning on going as far as trying to make it loadable as a C extension until there's an explicit request for that.
The Option pattern is common for Go and a familiar way to manage this. Though I'm surprised your LSP was having an issue. Are you using gopls? |
|
@copilot resolve the merge conflicts in this pull request |
|
I'm using the default editor extension in either VSCode or Zed which I assume is both gopls based. Do you see something different? |
Done. Applied the workflow security improvements from main (zizmor fixes: SHA pinning, |
…arch coupling to TestMain
…InstallTestServer
…equest in uaRoundTripper before mutating headers
…ring in parse error message
…ocsUrl to DocsURL
…est error, fix temp dir leak and double-close
…ir on create failure
…itize PR title input
…ase, close body before non-200 return in fetch
… empty PkgInfo, hint --pre for prerelease versions
… filter condition
…eturns VersionInfo, deduplicate test boilerplate
…test helpers, add deprecation tags
…n auth, fix SignedByColumnar deprecation
Agent-Logs-Url: https://github.com/columnar-tech/dbc/sessions/8f90f49a-b5ef-45bd-8c7a-4330246deb77 Co-authored-by: zeroshade <555095+zeroshade@users.noreply.github.com>
6033586 to
294956d
Compare
|
Interesting, I see the same thing you are. That's odd, though i don't need to assume the |
|
Thanks for checking. |
Summary
This branch refactors the
dbcpackage from a package-global,init()-heavy design to a clean library API:Clienttype with functional options (WithHTTPClient,WithRegistries,WithBaseURL,WithUserAgent,WithCredential,WithAuthFromFilesystem) replacing package-level singletonsdbcandconfigpackages —import "github.com/columnar-tech/dbc"now pulls zero bubbletea/lipgloss/bubbles transitive deps; widgets moved tocmd/dbcinit()side effects eliminated — no disk writes, nohttp.DefaultClientmutation at import time; lazysync.Once-based setup only triggered on first network callGetDriverList,DefaultClient) with forwarding shims for backward compatibility; CLI migrated to useClientinternallyNew Client API
Testing
All tests pass (
go test ./...). Branch reviewed and passed by automated code review (27 commits, roborev job 1027).