Skip to content
This repository has been archived by the owner on Mar 25, 2024. It is now read-only.

feat: impl Clone for Config struct #15

Merged
merged 1 commit into from
Apr 27, 2023
Merged

feat: impl Clone for Config struct #15

merged 1 commit into from
Apr 27, 2023

Conversation

markphelps
Copy link
Contributor

I want to be able to clone a config object to use for multiple clients, ie:

    let flipt_config = flipt::Config::default();

    flipt::meta::MetaClient::new(flipt_config)?
        .info()
        .get()
        .await?;

    let flipt_client = flipt::api::ApiClient::new(flipt_config)?;

Since the clients take ownership of the config argument, the only (easy) way I can see to do this is to implement Clone for Config and then .clone() it for each client I want to use.

Im not sure if this is the 'best' or idiomatic way to do this though.. so feel free to push back @brettbuddin

@markphelps markphelps requested a review from a team as a code owner April 26, 2023 18:06
@markphelps markphelps requested a review from brettbuddin April 26, 2023 18:40
@brettbuddin
Copy link
Contributor

I'll give this some 👀 this evening. I think we can do one better but I need to look through the struct's shape first.

Copy link
Contributor

@brettbuddin brettbuddin left a comment

Choose a reason for hiding this comment

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

With this you'll need to call clone() outside of the constructor unless you borrow the Config in ApiClient::new. I would recommend not borrowing and keep the contract as is.

Since you're copying anyway, it seems best to let the caller own when they want that to happen. This ensures that that the (likely?) common case of having a single config and client will result in no copies, and the more complicated cases like you've describe are possible.

Adjusting your example, you'll have something like this:

let flipt_config = flipt::Config::default();

// Clones flipt_config and moves it into MetaClient::new
let meta_client = flipt::meta::MetaClient::new(flipt_config.clone())?
    .info()
    .get()
    .await?;

// Moves the original into ApiClient::new
let flipt_client = flipt::api::ApiClient::new(flipt_config)?;

In a perfect world you'd be able to implement Copy and make that all automatic, but none of the fields of Config have Copy. Clone it is then.

Copy link
Member

@GeorgeMac GeorgeMac left a comment

Choose a reason for hiding this comment

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

What @brettbuddin said.

@markphelps
Copy link
Contributor Author

So did I do good Crab? 🦀

@markphelps markphelps merged commit 4d3f81f into main Apr 27, 2023
@markphelps markphelps deleted the config-clone branch April 27, 2023 12:13
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants