Draft
Conversation
davecheney
force-pushed
the
dfc/split-cmd-tap
branch
2 times, most recently
from
b76239f to
21c88bc
Compare
jcalabro
reviewed
Jan 27, 2026
jcalabro
approved these changes
Jan 27, 2026
Member
jcalabro
left a comment
jcalabro
left a comment
There was a problem hiding this comment.
one question, otherwise I'm down with the pattern
I was heavily using ./internal in https://github.com/bluesky-social/indigo/tree/jc/cask
This PR explores seperating the cmd/tap, the binary that holds main(), from tap, the service (or perhaps component if one was so keen as to run tap as part of a larger application). Some plusses; main.NewTap becomes tap.New(), main.TapConfig becomes tap.Config, which is always pleasing to see. The downside is the shadowing of `tap, err := tap.New(...)`, which is perfectly cromulent, but the shaddowing may not be to everyone's taste. This could be resolved by `t, err := tap.New(...)` if you're ok with the brevity, or `theTap, err := tap.New(...)` if you feel like pushing the boat out.
davecheney
force-pushed
the
dfc/split-cmd-tap
branch
from
21c88bc to
cc0c8d0
Compare
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
This PR explores seperating the cmd/tap, the binary that holds main(), from tap, the service (or perhaps component if one was so keen as to run tap as part of a larger application).
Some plusses;
main.NewTapbecomestap.New(),main.TapConfigbecomestap.Config, which is always pleasing to see. The downside is the shadowing oftap, err := tap.New(...), which is perfectly cromulent, but the shadowing may not be to everyone's taste. This could be resolved byt, err := tap.New(...)if you're ok with the brevity, ortheTap, err := tap.New(...)if you feel like pushing the boat out.This also gives names to the other tap components, eg,
tap.Crawlerwhich is a hint that it may not belong in the tap package. Another clue is tap.Crawler (crawler.go) doesn't import any of the usual suspects like../tap/models. This is something to explore in the future.Not really for review, more for discussion