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

Use PhantomData over a string for query error #1

Open
wants to merge 1 commit into
base: more-informations-for-query-errors
Choose a base branch
from

Conversation

nicopap
Copy link

@nicopap nicopap commented Aug 19, 2023

Objective

  • Reduce space requirement of query errors.

Solution

  • Use PhantomData over a &'static str
  • Use the thiserror::Error macro to not manually implement Error and Display, bonus is it makes implementation more concise.
  • Use a struct + enum instead of an enum with many branches that all have the same fields for QueryEntityError.

Note that it doesn't make sense to verify that the type is correct in the error message since it's guarenteed at compile time now.

What is left to do

  • I adjusted the unit tests, but not the doc tests. Need to fix them.
  • The same process should be repeated for QuerySingleError
  • Remove the Vec<ComponentMismatch> from ErrorType, it is a huge blocker.
  • It should be possible to replace the QueryEntityError<ErrInfo<Q, F>> by a QueryEntityError<Q, F>. You could define a type alias so that QueryEntityError<Q::ReadOnly, F> can be used as ReadQueryEntityError<Q, F>

Note that I spent an hour preparing this :)

ErrorTypeInfo(PhantomData)
}
}
impl<Q: WorldQuery, F: ReadOnlyWorldQuery> fmt::Debug for ErrorTypeInfo<Q, F> {
Copy link
Author

Choose a reason for hiding this comment

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

Suggested change
impl<Q: WorldQuery, F: ReadOnlyWorldQuery> fmt::Debug for ErrorTypeInfo<Q, F> {
impl<Q: WorldQuery, F: ReadOnlyWorldQuery> fmt::Display for ErrorTypeInfo<Q, F> {

I'm not exactly sure how this compiles. at line 1381

#[error("Error querying for {entity:?} in query {error_type_info}: {kind}")]

It calls the display implementation of ErrorTypeInfo, yet there is no such impl.

@nicopap
Copy link
Author

nicopap commented Aug 23, 2023

@Selene-Amanita bump.

Note that the "what is left to do" section is not things I will do.

@Selene-Amanita
Copy link
Owner

I'm a bit busy with work this week but I promise I will look at it, thank you!

@nicopap
Copy link
Author

nicopap commented Sep 18, 2023

@Selene-Amanita bump again.

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.

2 participants