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

PkvStore Resource fix #16

Merged
merged 5 commits into from Nov 14, 2022
Merged

Conversation

Aultus-defora
Copy link
Contributor

@Aultus-defora Aultus-defora commented Nov 13, 2022

My attempt at fixing PkvStore not being a resource. Not sure if it is possible to add something smaller than bevy_ecs to dependencies. The error is PkvStore: bevy::prelude::Resource is not satisfied for Res<PkvStore>, ResMut<PkvStore>, ect.

my attempt at fixing PkvStore not being a resource
fix gramma in a comment
@johanhelsing
Copy link
Owner

Thanks for the PR!

I wonder if it'd be possible to make bevy an optional dependency?

Something like:

#[derive(Debug)]
#[cfg_attr(feature = "bevy", derive(Resource))]
struct PkvStore {

Also, not sure what the practical difference is between importing bevy_ecs vs. importing bevy with default-features = false and then enable the features needed? I usually do the latter, but it would be good to know whether one approach has advantages over the other.

@johanhelsing johanhelsing added the enhancement New feature or request label Nov 13, 2022
@Aultus-defora
Copy link
Contributor Author

Aultus-defora commented Nov 13, 2022

I am not sure what feature(s) is(are) enough to make #[derive(Resource)] work.

@Aultus-defora
Copy link
Contributor Author

Aultus-defora commented Nov 13, 2022

Now I use features to limit the Resource use, used cargo clippy to make linting good, also updated table — in 0.9 one has to at the very least create new struct with Resource to have PkvStore inside of a resource which is a hassle, this should solve that issue. P. S. I think adding bevy with no features as a dependency might be enough for it to work but I suspect bevy_ecs would be a smaller dependency than bevy

README.md Outdated Show resolved Hide resolved
@johanhelsing
Copy link
Owner

Nice, this was exactly what I had in mind. Just a small error in the version table.

Co-authored-by: Johan Klokkhammer Helsing <johanhelsing@gmail.com>
@Aultus-defora
Copy link
Contributor Author

Aultus-defora commented Nov 13, 2022

I merged the version table fix. Also confirmed that the #[cfg_attr(feature = "bevy", derive(Resource))] works.

Copy link
Owner

@johanhelsing johanhelsing left a comment

Choose a reason for hiding this comment

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

Aside from fixing the linting error. with cfg attributes, I find it cleaner to just fully qualify types

src/lib.rs Outdated Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants