-
Notifications
You must be signed in to change notification settings - Fork 32
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Renaming RegistryInterface to Registry and hiding impl details #88
Conversation
spectator/registry.go
Outdated
logger logger.Logger | ||
loggerMutex sync.RWMutex | ||
} | ||
|
||
// NewRegistryConfiguredBy loads a new Config JSON file from disk at the path specified. | ||
func NewRegistryConfiguredBy(filePath string) (*Registry, error) { | ||
func NewRegistryConfiguredBy(filePath string) (Registry, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hmm, if you updated this signature to take an optional logger argument and then assigned that to the config, it would obviate the need for a mutable logger on the Registry itself, further simplifying that interface.
func NewRegistryConfiguredBy(filePath string) (Registry, error) { | |
func NewRegistryConfiguredBy(filePath string, logger Logger) (Registry, error) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Removed the option to create a Registry from a file-based config. Thanks for the recommendation 🙌
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Nice, this should let you shrink surface area of the registry interface too!
…on. Removed SetLogger function from Registry API
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The overall change looks good, but I think we should add a mention of the SPECTATOR_OUTPUT_LOCATION
env var to the README. The use case for this one is configuring no-op writers in CI environments as a fall-back strategy.
I think it's already there: https://github.com/Netflix/spectator-go/blob/main/README.md?plain=1#L132 |
Whoops, I lost track of it in the diffs. Thank you! |
Renamed
RegistryInterface
toRegistry
.Reduced visibility for
spectatordRegistry
.Meant to reduce library API.