Skip to content
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

client: added the ability to validate the configuration #197

Closed
wants to merge 1 commit into from

Conversation

dwagin
Copy link
Contributor

@dwagin dwagin commented Aug 23, 2022

For recreating the client, need the ability to check the configuration separately.

in continuation #195.

Copy link
Owner

@twmb twmb left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think that since NewClient does more initialization and validation than this, the Validate function here isn't comprehensive enough to all the errors that could be returned. Also, I think the name needs to be a bit more specific, either ValidateOpts or CheckNewClient or ValidateNewClient or something.

I'd split the two goroutines started in NewClient into something like func (*Client) start(), add func (*Client) init(...Opt) error, and then have NewClient be init(opt...); errcheck; start() and ValidateOpts to be just return init().

Although this runs the risk someday of accidentally adding a goroutine in the middle of NewClient, so then add a doc line warning maintainers in a year not to add goroutines.

Also, since 1.7 has just released, this will probably wait a bit until a few more features for 1.8 stack up, perhaps a month or three in the worst case.

@twmb twmb mentioned this pull request Sep 27, 2022
8 tasks
@twmb
Copy link
Owner

twmb commented Oct 6, 2022

I've gone ahead and applied my suggestions above and committed this in 7ffb6e1 (co-author commit). Closing this PR now as duplicate -- this will be released in 1.8

@twmb twmb closed this Oct 6, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants