Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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] - Implement
WorldQuery
derive macro #2713[Merged by Bors] - Implement
WorldQuery
derive macro #2713Changes from all commits
ba31c9b
3cb3fb6
8574a8a
a717b7b
f67876a
005fbf6
ba68958
b7845d9
0a8ba72
cfe8db3
a2f3d3b
27b1155
96f6043
10510f2
File filter
Filter by extension
Conversations
Jump to
There are no files selected for viewing
Large diffs are not rendered by default.
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'm surprised by this limitation: tuple structs are named structs, just with dumb field names of
0
,1
etc.See rust-lang/rust-clippy#7985
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.
Oh, cool, I forgot about this. This indeed makes it a lot easier to support tuple structs
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've just tried it - unfortunately, it doesn't make it much easier to implement a macro.
Tuple structs can't be declared like this:
Also,
Ident::new("0", Span::call_site());
panics.Without this, adding tuple structs support won't be free and will likely produce much more convoluted conditions inside the macro code. Btw, our
Bundle
derive doesn't support tuple structs either, so I'd prefer to leave it as is for now for consistency and simplicity sake.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.
This is fine by me for now. Although do note that #3634 exists.
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.
This should prevent users forgeting to implement
FetchedItem
and thus makeWorldQuerys
that don't support the derive.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.
Yeah, I tried to do this, but unfortunately, it won't work for filters, as their
Item
isbool
. Andbool
can mean absolutely any filter...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.
Can we
impl FetchedItem for bool
?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.
@alice-i-cecile I'm afraid we can't.
<<With<T> as WorldQuery>::Fetch as Fetch>::Item
corresponds tobool
. So doesWithout<T>
. As it's not a one-to-one relation for filters, I think there's no way to implement it.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.
Maybe we'll be able to do it one day if we ban using filters as regular queries' items.
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.
Ok, I guess we can do it like so: e65d274
It's obviously useless for determining a filter type but allows us to add
FetchedItem
bound toFetch::Item
.