Start adding support for FabricClientSettings and FabricSecurityCredentials #139
Conversation
| .expect("failed to cast fabric client to IFabricClientSettings2"); | ||
| if let Some(desired_settings) = client_settings { | ||
| desired_settings | ||
| .apply(&setting_interface) |
There was a problem hiding this comment.
IMO the combining of the setting value should be the responsibility of the user. U can provide api to load existing values.
There was a problem hiding this comment.
Sure, but I'm not doing the combining of the setting value in this PR at all - it just is applying the settings, if provided.
The "what does that mean, do we combine settings" isn't in this PR at all.
I could make two methods - with_client_settings_overrides and with_client_settings, if we want to expose the capability to not get defaults from the client. But they can also always fall back to the COM API for that sort of advanced scenario?
Or are you saying you'd rather client settings and credentials not be part of the builder, but instead be &mut self methods on FabricClient and the individual clients as well?
There was a problem hiding this comment.
with_client_settings.
FabricClientSettings should have fn to load existing settings.
| @@ -34,13 +38,22 @@ pub use notification::ServiceNotification; | |||
| #[cfg(test)] | |||
| mod tests; | |||
|
|
|||
| #[non_exhaustive] | |||
| #[derive(Debug)] | |||
| pub enum FabricClientCreationError { | |||
There was a problem hiding this comment.
Will do in follow up PR
| // Copyright (c) Microsoft Corporation. All rights reserved. | ||
| // Licensed under the MIT License (MIT). See License.txt in the repo root for license information. | ||
| // ------------------------------------------------------------ | ||
| #![deny(unsafe_op_in_unsafe_fn)] |
There was a problem hiding this comment.
Where are denies needed?
There was a problem hiding this comment.
These are just me tightening up warnings in new code. unsafe_op_in_unsafe_fn requires discrete unsafe blocks inside unsafe functions to better clarify where unsafeness is (believe this becomes on by default in edition 2024)
undocumented_unsafe_block makes me write // SAFETY comments
There was a problem hiding this comment.
I don't see unsafe code in this mod in this PR. Assme it will be used in the next.
I'd like to have support for ergonomic (i.e. safe) versions of FABRIC_CLIENT_SETTINGS and FABRIC_SECURITY_CREDENTIALS
This makes necessary error handling changes (that are breaking), as FabricClientBuilder::build() now can error if there are invalid settings. It's a small step towards #140