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

Add clone to builder state types #379

Merged
merged 1 commit into from
Oct 12, 2022

Conversation

JunichiSugiura
Copy link
Contributor

@JunichiSugiura JunichiSugiura commented Sep 28, 2022

This PR is to add Clone impl to both DefaultState and AsyncState. This allows ConfigBuilder to be fully clonable. My goal is to make bevy plugin that inserts ConfigBuilder as resource so that developers can query them from systems and modify (e.g. configure additional source file etc.).

I'm not 100% sure this is a right approach but it satisfied my use case for now. Let me know if there's other way to make it work.

Example

Without clone

🦄  cargo run --example cli_config
   Compiling dip v0.1.0 (/Users/js/projects/github.com/JunichiSugiura/dip)
error[E0507]: cannot move out of dereference of `dip::prelude::ResMut<'_, ConfigBuilder<DefaultState>>`
  --> examples/cli/config/main.rs:17:16
   |
17 |     *builder = builder.add_source(File::with_name("examples/cli/config/config/development"));
   |                ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ move occurs because value has type `ConfigBuilder<DefaultState>`, which does not implement the `Copy` trait

Copy link
Collaborator

@matthiasbeyer matthiasbeyer left a comment

Choose a reason for hiding this comment

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

I don't see why the states couldn't be Clone, so yes, I think we can merge this (if CI is green of course 😆 ).

@JunichiSugiura
Copy link
Contributor Author

What's the status on this PR?

@matthiasbeyer
Copy link
Collaborator

Ah, thanks for pinging me!

@matthiasbeyer matthiasbeyer merged commit 655a02d into mehcode:master Oct 12, 2022
@JunichiSugiura JunichiSugiura deleted the clonable-builder-state branch October 13, 2022 08:10
@JunichiSugiura
Copy link
Contributor Author

Thanks for the merge !

@JunichiSugiura
Copy link
Contributor Author

@matthiasbeyer Hi, what's the plan for publishing a new version including this PR? This has been a blocker to bump my lib.

@matthiasbeyer
Copy link
Collaborator

Hm. Can this be considered a backwards compatible fix? I am not sure. If it is, I could release a patchlevel version update.

@JunichiSugiura
Copy link
Contributor Author

@matthiasbeyer This PR just adds Clone trait so the old code still works without any changes, I think.

@matthiasbeyer
Copy link
Collaborator

Submitted backport in #398

Will release 0.13.3 with this patch shortly.

@JunichiSugiura
Copy link
Contributor Author

@matthiasbeyer Thank you so much 🙏

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