-
-
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
FilteredEntityRef
conversions
#11838
FilteredEntityRef
conversions
#11838
Conversation
@@ -189,6 +189,58 @@ impl<'a> From<&'a EntityMut<'_>> for EntityRef<'a> { | |||
} | |||
} | |||
|
|||
impl<'a> TryFrom<FilteredEntityRef<'a>> for EntityRef<'a> { | |||
type Error = (); |
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.
Does worth it adding an error for this conversion, so users may know why it failed, something like:
pub enum EntityRefAccessError {
NoReadAll,
NoWriteAll,
}
impl fmt::Display for EntityRefAccessError {
fn fmt(&self, f: &mut fmt::Formatter<'_>) -> fmt::Result {
match self {
Self::NoReadAll=> write!(f, "The given FilteredEntityRef has no read access on all entities."),
Self::NoWriteAll=> write!(f, "The given FilteredEntityRef has no write access on all entities."),
}
}
}
impl std::error::Error for IdentifierError {}
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.
Added the error enum, went with slightly different names to match the naming conventions for existing TryFrom
errors, and also for variants of this existing error type
type Error = (); | ||
|
||
fn try_from(value: FilteredEntityMut<'a>) -> Result<Self, Self::Error> { | ||
if value.access.has_read_all() && value.access.has_write_all() { |
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.
NIT: AFAIK you don't have to check for read all access, since it's impossible to have write_all access without read_all
if value.access.has_read_all() && value.access.has_write_all() { | |
if value.access.has_write_all() { |
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'd like to keep this in as a failsafe.
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.
Is this guarantee something that we want to match the semantics of &
and &mut
in Rust, to keep it straightforward, or is it possible that we might want to have write-only access in the future?
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 think it's possible that we have write only access to certain data in the future. And more importantly, users can currently do this on their own.
Objective
Right now, it's a bit cumbersome to write code that simultaneously deals with both
FilteredEntityRef
/EntityRef
or withFilteredEntityMut
/EntityMut
. This PR aims to make it easier by allowing conversions (both infallible and fallible) between them.Solution
Changelog
From
)EntityRef<'a>
→FilteredEntityRef<'a>
&'a EntityRef
→FilteredEntityRef<'a>
EntityMut<'a>
→FilteredEntityRef<'a>
&'a EntityMut
→FilteredEntityRef<'a>
EntityWorldMut<'a>
→FilteredEntityRef<'a>
&'a EntityWorldMut
→FilteredEntityRef<'a>
EntityMut<'a>
→FilteredEntityMut<'a>
&'a mut EntityMut
→FilteredEntityMut<'a>
EntityWorldMut<'a>
→FilteredEntityMut<'a>
&'a mut EntityWorldMut
→FilteredEntityMut<'a>
TryFrom
)FilteredEntityRef<'a>
→EntityRef<'a>
&'a FilteredEntityRef
→EntityRef<'a>
FilteredEntityMut<'a>
→EntityRef<'a>
&'a FilteredEntityMut
→EntityRef<'a>
FilteredEntityMut<'a>
→EntityMut<'a>
&'a mut FilteredEntityMut
→EntityMut<'a>