add helping log when trying to access an entity that does not exist #1712
Conversation
alice-i-cecile
added
C-Code-Quality
alice-i-cecile
added
the
S-Ready-For-Final-Review
| let location = match self.entities.get(entity) { | ||
| Ok(entity_location) => entity_location, | ||
| Err(err) => { | ||
| warn!("Error getting entity {:?}: {:?}", entity, err,); |
There was a problem hiding this comment.
| warn!("Error getting entity {:?}: {:?}", entity, err,); | |
| warn!("Error getting entity {:?}: {:?}", entity, err); |
I think this trailing comma is here by mistake?
| let location = match self.entities.get(entity) { | ||
| Ok(entity_location) => entity_location, | ||
| Err(err) => { | ||
| warn!("Error getting entity {:?}: {:?}", entity, err,); |
There was a problem hiding this comment.
I think warn! is a too strong log level. The user may very well just not care about if the entity exists or not. I think it would be better to return Result<EntityRef, GetEntityError> where GetEntityError has a Debug implementation that shows all this information.
There was a problem hiding this comment.
I agree. This method feels too "low level" and could be called in too many different situations to call this case a warning / make it unrecoverable. I'd prefer to preserve the simplicity of the Option return type (like std's collection types) if possible, but if we really need to expose multiple "failure" states, it probably makes sense to just return an error here.
There was a problem hiding this comment.
I... can't remember why I wanted a warn here :(
I could weakly argue that the user should not ignore this, but the Option already achieve that.
For the error reason, I think knowing it can help the user debug issues looking at the log but is not very useful from the code. Should I change the log to info or debug?
There was a problem hiding this comment.
I honestly don't want to log at all here. If some subset of users need to care about the various reasons an entity might not exist, then imo we need to return a Result here.
Noticed in #1707
This adds a warn log when accessing an entity that doesn't exist, and the reason (doesn't exist yet or already despawned)