Skip to content

feat: add overridable_capsule side effect#64

Merged
GregoryConrad merged 1 commit intomainfrom
overridable-capsules
Aug 4, 2024
Merged

feat: add overridable_capsule side effect#64
GregoryConrad merged 1 commit intomainfrom
overridable-capsules

Conversation

@GregoryConrad
Copy link
Copy Markdown
Owner

See #62

@GregoryConrad
Copy link
Copy Markdown
Owner Author

CC @busslina, I'll publish a new rearch-effects with this addition in the next couple hours

@GregoryConrad GregoryConrad merged commit 5b5391f into main Aug 4, 2024
@GregoryConrad GregoryConrad deleted the overridable-capsules branch August 4, 2024 17:06
@busslina
Copy link
Copy Markdown

busslina commented Aug 5, 2024

I just migrated 90% of my code to use this abstraction (which is great) but I have an issue. I have a lib level function that overrides two capsules and I need to use DynCapsuleHolder. What do you think about making it public?

pub fn override_capsules(
    app_name_capsule: DynCapsuleHolder<String>,
    app_config_file_capsule: DynCapsuleHolder<String>,
) {
    // App name
    CAPSULE_CONTAINER.read(app_name_overridable_capsule).1(app_name_capsule);

    // App config file
    CAPSULE_CONTAINER
        .read(app_config_file_overridable_capsule)
        .1(app_config_file_capsule);
}

@GregoryConrad
Copy link
Copy Markdown
Owner Author

@busslina I’d recommend using the new effect I added to rearch-effects (overridable_capsule) instead of the code snippet I gave before; it’s a little more robust.

But regardless, you can just pass in the capsules themselves (using generics or the impl Trait syntax). The new OverridableCapsule and the DynCapsuleHolder I gave in the code snippet earlier can be constructed from regular Capsules.

@busslina
Copy link
Copy Markdown

busslina commented Aug 5, 2024

I’d recommend using the new effect I added to rearch-effects (overridable_capsule) instead of the code snippet I gave before; it’s a little more robust.

I'm using it:

// -------------------------------------------------------------------------
// App name
// -------------------------------------------------------------------------
pub fn app_name_capsule(mut handle: CapsuleHandle) -> String {
    let capsule = handle.get.as_ref(app_name_overridable_capsule).clone();
    handle.get.as_ref(capsule).to_owned()
}

pub fn app_name_overridable_capsule(handle: CapsuleHandle) -> OverridableCapsule<String> {
    handle
        .register
        .register(overridable_capsule(app_name_capsule_default))
}

fn app_name_capsule_default(_: CapsuleHandle) -> String {
    panic!("App name capsule must be override");
}
// -------------------------------------------------------------------------

Just that I have a lib helper function that handles the override initialization. The function content is outdated. I was just focusing on the function signature that uses DynCapsuleHolder. Anyways, I will try to refactor it to use impl Capsule, but seems maybe util to be able to use DynCapsuleHolder.

@busslina
Copy link
Copy Markdown

busslina commented Aug 5, 2024

Okay, I think I'm wrong, and I can't use DynCapsuleHolder anymore with this new abstraction.

@busslina
Copy link
Copy Markdown

busslina commented Aug 5, 2024

Solved:

pub fn override_capsules(
    app_name_capsule: impl Capsule<Data = String> + CData,
    app_config_file_capsule: impl Capsule<Data = String> + CData,
) {
    // App name
    CAPSULE_CONTAINER
        .read(app_name_overridable_capsule)
        .set(app_name_capsule);

    // App config file
    CAPSULE_CONTAINER
        .read(app_name_overridable_capsule)
        .set(app_config_file_capsule);
}

Good job! :)

@GregoryConrad
Copy link
Copy Markdown
Owner Author

Solved:

pub fn override_capsules(
    app_name_capsule: impl Capsule<Data = String> + CData,
    app_config_file_capsule: impl Capsule<Data = String> + CData,
) {
    // App name
    CAPSULE_CONTAINER
        .read(app_name_overridable_capsule)
        .set(app_name_capsule);

    // App config file
    CAPSULE_CONTAINER
        .read(app_name_overridable_capsule)
        .set(app_config_file_capsule);
}

Good job! :)

Looks about right! Only recommendation is to change CData to Sync, since that's the only extra bound you need.

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