-
-
Notifications
You must be signed in to change notification settings - Fork 324
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
add moving with_default_namespace Client setter - fixes #543 #544
Conversation
Yeah, I forgot about this. I don't think this is bad: let config = Config::infer().await?;
let https = config.native_tls_https_connector()?;
let client = Client::new(
ServiceBuilder::new()
.layer(config.base_uri_layer())
.option_layer(config.auth_layer()?)
.service(hyper::Client::builder().build(https)),
config.default_namespace,
); It's a breaking change, but this is not widely used yet and the fix is trivial. |
Yeah, that is true. But it's also possible that the user is just puts "default" there anyway. We don't really have full safety here either way. If we had more properties required (like extensions), we could have put let client = Client::new(service).with_defaults(config);
let client = Client::new(service, config); but right now that's even more wasteful allocation-wise. Probably thinking about this for more than I should. The explicit I'll tweak the PR |
Yeah, my point was "forgetting" about this setter. If the default namespace was a parameter, then it's impossible to forget. I'm hoping |
Yeah, I think that's the right trade-off as well now. PR updated. |
Co-authored-by: kazk <kazk.dev@gmail.com>
Follow up to #534 after custom client support in #540.
add movingClient::with_default_namespace
setterdefault_namespace
as anInto<String>
arg inClient::new
Config::default_ns
toConfig::default_namespace
Client::new_with_default_namespace
alloc)