-
-
Notifications
You must be signed in to change notification settings - Fork 3.6k
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
[Merged by Bors] - Add attribute to ignore fields of derived labels #5366
Changes from all commits
6262c42
2e12912
cc895ee
ae0769d
dc56e1d
66d687a
a633baf
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change | ||||
---|---|---|---|---|---|---|
@@ -0,0 +1,62 @@ | ||||||
use std::marker::PhantomData; | ||||||
|
||||||
use bevy_ecs::prelude::*; | ||||||
|
||||||
fn main() { | ||||||
// Unit labels are always equal. | ||||||
assert_eq!(UnitLabel.as_label(), UnitLabel.as_label()); | ||||||
|
||||||
// Enum labels depend on the variant. | ||||||
assert_eq!(EnumLabel::One.as_label(), EnumLabel::One.as_label()); | ||||||
assert_ne!(EnumLabel::One.as_label(), EnumLabel::Two.as_label()); | ||||||
|
||||||
// Labels annotated with `ignore_fields` ignore their fields. | ||||||
assert_eq!(WeirdLabel(1).as_label(), WeirdLabel(2).as_label()); | ||||||
|
||||||
// Labels don't depend only on the variant name but on the full type | ||||||
assert_ne!( | ||||||
GenericLabel::<f64>::One.as_label(), | ||||||
GenericLabel::<char>::One.as_label(), | ||||||
); | ||||||
} | ||||||
|
||||||
#[derive(SystemLabel)] | ||||||
pub struct UnitLabel; | ||||||
|
||||||
#[derive(SystemLabel)] | ||||||
pub enum EnumLabel { | ||||||
One, | ||||||
Two, | ||||||
} | ||||||
|
||||||
#[derive(SystemLabel)] | ||||||
#[system_label(ignore_fields)] | ||||||
pub struct WeirdLabel(i32); | ||||||
|
||||||
#[derive(SystemLabel)] | ||||||
pub enum GenericLabel<T> { | ||||||
One, | ||||||
#[system_label(ignore_fields)] | ||||||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more.
Suggested change
Bikeshedding: I think this is marginally clearer, less verbose and more idiomatic. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. To me, that implies that we're ignoring the entire variant, but we're only ignoring the fields. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Oh I see 🤔 Yeah I think you're right, but we should make sure to make this clear in the docs. Weird to think that you could set your enum label to the phantomdata variant... |
||||||
Two(PhantomData<T>), | ||||||
} | ||||||
|
||||||
// FIXME: this should be a compile_fail test | ||||||
/*#[derive(SystemLabel)] | ||||||
pub union Foo { | ||||||
x: i32, | ||||||
}*/ | ||||||
|
||||||
// FIXME: this should be a compile_fail test | ||||||
/*#[derive(SystemLabel)] | ||||||
#[system_label(ignore_fields)] | ||||||
pub enum BadLabel { | ||||||
One, | ||||||
Two, | ||||||
}*/ | ||||||
|
||||||
// FIXME: this should be a compile_fail test | ||||||
/*#[derive(SystemLabel)] | ||||||
pub struct BadLabel2 { | ||||||
#[system_label(ignore_fields)] | ||||||
x: (), | ||||||
}*/ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
These tests are critical, but won't be run in CI. I actually think that these should be moved to doc tests on
SystemLabel
.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I agree, but we can't actually add doctests to label types until #5367