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

Switched specs-derive to use Component in scope #467

Merged
merged 3 commits into from
Sep 22, 2018

Conversation

AnneKitsune
Copy link
Contributor

@AnneKitsune AnneKitsune commented Sep 13, 2018

"Works on my pc".

So with this you need to have Component in scope for the derive to find it and use it. This allows to use it in projects without having to have specs as a hard dependency. <3

Also it might fix the issue with re-exporting the macro, but I didn't test that.


This change is Reviewable

@randomPoison
Copy link
Member

I believe this is a breaking change, isn't it? Any code that uses #[derive(Component)] and doesn't explicitly bring Component into scope is going to fail.

As a user of specs (by way of Amethyst), I also don't love the prospect of having to manually import Component into scope for every file where I use #[derive(Component)]. This approach is pretty counter to the common pattern in custom derives to always use the full path internally, which means its also potentially confusing for new users who are expecting #[derive(Component)] to work the way most other derives do.

For the record, I'm not against this change. I think being able to derive Component, even if I have to manually bring it into scope, would be an improvement. But I also think we're going to eventually want a more robust solution.

@AnneKitsune
Copy link
Contributor Author

Of course, but before this change you needed to manually add specs to your Cargo.toml and add a "extern crate specs". In the case of amethyst where its already re-exported (amethyst::core::ecs), you end up with two versions of specs. If you are not careful, you'll get two different versions and get hard to track errors.

But yes a way of doing it with absolute paths would be nice indeed.
By the way, if you check at the source of the derive, in the lines I didn't touch DenseVecStorage is not imported as an absolute path either. https://github.com/jojolepro/specs/blob/derive/specs-derive/src/lib.rs#L58

:D

@randomPoison
Copy link
Member

You know what, I feel like I've run into that before and been annoyed about having to manually import DenseVecStorage 🤔

Copy link
Member

@randomPoison randomPoison left a comment

Choose a reason for hiding this comment

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

I'm in favor of this as a first step towards being able to re-export the custom derives in other crates. We can always make improvements later 👍

Copy link
Member

@Aceeri Aceeri left a comment

Choose a reason for hiding this comment

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

Is a breaking change if they don't have Component imported, but seems fine.

Copy link
Member

@torkleyy torkleyy left a comment

Choose a reason for hiding this comment

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

Please bump the version of specs-derive. We might need to hold this off until we release Specs 0.13.

@AnneKitsune
Copy link
Contributor Author

@torkleyy done

@Xaeroxe
Copy link
Member

Xaeroxe commented Sep 21, 2018

@Jojolepro tests failed, can you update the version usage as well?

@Xaeroxe
Copy link
Member

Xaeroxe commented Sep 22, 2018

bors r+

bors bot added a commit that referenced this pull request Sep 22, 2018
467: Switched specs-derive to use Component in scope r=Xaeroxe a=jojolepro

"Works on my pc".

So with this you need to have `Component` in scope for the derive to find it and use it. This allows to use it in projects without having to have specs as a hard dependency. <3

Also it might fix the issue with re-exporting the macro, but I didn't test that.

<!-- Reviewable:start -->
---
This change is [<img src="https://reviewable.io/review_button.svg" height="34" align="absmiddle" alt="Reviewable"/>](https://reviewable.io/reviews/slide-rs/specs/467)
<!-- Reviewable:end -->


Co-authored-by: Joël Lupien (Jojolepro) <[email protected]>
@bors
Copy link
Contributor

bors bot commented Sep 22, 2018

Build succeeded

@bors bors bot merged commit 9e0c03b into amethyst:master Sep 22, 2018
@torkleyy
Copy link
Member

@Xaeroxe We should've held the merge off until 0.13..

@Xaeroxe
Copy link
Member

Xaeroxe commented Sep 22, 2018

We did? I'm planning a 0.13 soon.

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.

5 participants