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.

Sign up for GitHub

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

Jump to bottom

[Merged by Bors] - WorldQuery derive macro now respects visibility #4125

Closed
wants to merge 3 commits into from
Closed

[Merged by Bors] - WorldQuery derive macro now respects visibility #4125

wants to merge 3 commits into from

Conversation

ItsDoot
Copy link
Contributor

@ItsDoot ItsDoot commented Mar 6, 2022

Objective

Fixes #4122.

Solution

Inherit the visibility of the struct being derived for the xxItem, xxFetch, xxState structs.

@github-actions github-actions bot added the S-Needs-Triage This issue needs to be labelled label Mar 6, 2022
@ItsDoot
Copy link
Contributor Author

ItsDoot commented Mar 6, 2022

Additional Notes

Given this derive macro generates additional structs, there isn't a way currently to annotate those structs with additional attributes like documentation comments. This leaves me with warnings from #![warn(missing_docs)]. Should these generated structs have documentation comments that just link to the main struct, or maybe even have some mechanism to define attributes on the generated structs?

@ghost ghost added A-ECS Entities, components, systems, and events C-Code-Quality A section of code that is hard to understand or change and removed S-Needs-Triage This issue needs to be labelled labels Mar 6, 2022
@alice-i-cecile
Copy link
Member

Should these generated structs have documentation comments that just link to the main struct

IMO yes, along with a link to [WorldQuery] and the derive macro used. Or alternatively, I think we could hide these structs from the docs and it would silence the warning?

alice-i-cecile
alice-i-cecile approved these changes Mar 7, 2022
View reviewed changes
Copy link
Member

@alice-i-cecile alice-i-cecile left a comment

Choose a reason for hiding this comment

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

Code checks out. Nice small fix.

@alice-i-cecile alice-i-cecile added the S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it label Mar 7, 2022
@DJMcNab
Copy link
Member

DJMcNab commented Mar 7, 2022

Yeah, these structs are implementation details. They should be doc(hidden). We should probably follow the lead of derive(SystemParam)

#4100 proposes to use an anonymous scope for them; however that could be problematic for WorldQuery if rust-analyzer doesn't like it.

@ItsDoot
Copy link
Contributor Author

ItsDoot commented Apr 1, 2022

Yeah, these structs are implementation details. They should be doc(hidden).

Apologies for the delay, I've added #[doc(hidden)] to the generated structs now.

@infmagic2047
Copy link
Contributor

I believe the Item struct should not be hidden, as it is the user-facing type.

@ItsDoot ItsDoot mentioned this pull request Apr 13, 2022
Closed
@alice-i-cecile alice-i-cecile added this to the Bevy 0.7 milestone Apr 13, 2022
IceSentry
IceSentry approved these changes Apr 13, 2022
View reviewed changes
Copy link
Contributor

@IceSentry IceSentry left a comment

Choose a reason for hiding this comment

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

It looks good to me, but I'm not super familiar with that code

@cart
Copy link
Member

cart commented Apr 13, 2022

I believe the Item struct should not be hidden, as it is the user-facing type.

I agree with this. Someone trying to specify the returned type from a query in code should be able to find it in the docs.

@cart @alice-i-cecile
Update crates/bevy_ecs/macros/src/fetch.rs
fc7e97a 
Co-authored-by: Alice Cecile <alice.i.cecile@gmail.com>
@cart
Copy link
Member

cart commented Apr 13, 2022

bors r+

bors bot pushed a commit that referenced this pull request Apr 13, 2022
@ItsDoot @cart
WorldQuery derive macro now respects visibility (#4125)
16133de 
## Objective

Fixes #4122.

## Solution

Inherit the visibility of the struct being derived for the `xxItem`, `xxFetch`, `xxState` structs.

Co-authored-by: Carter Anderson <mcanders1@gmail.com>
@bors
Copy link
Contributor

bors bot commented Apr 13, 2022

Pull request successfully merged into main.

Build succeeded:

@bors bors bot changed the title WorldQuery derive macro now respects visibility [Merged by Bors] - WorldQuery derive macro now respects visibility Apr 13, 2022
@bors bors bot closed this Apr 13, 2022
aevyrie pushed a commit to aevyrie/bevy that referenced this pull request Jun 7, 2022
@ItsDoot @cart @aevyrie
WorldQuery derive macro now respects visibility (bevyengine#4125)
a116ab2 
## Objective

Fixes bevyengine#4122.

## Solution

Inherit the visibility of the struct being derived for the `xxItem`, `xxFetch`, `xxState` structs.

Co-authored-by: Carter Anderson <mcanders1@gmail.com>
@james7132 james7132 mentioned this pull request Dec 4, 2022
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@alice-i-cecile alice-i-cecile alice-i-cecile approved these changes

@IceSentry IceSentry IceSentry approved these changes

Assignees
No one assigned
Labels
A-ECS Entities, components, systems, and events C-Code-Quality A section of code that is hard to understand or change S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it
Projects
None yet
Milestone
Bevy 0.7
Development

Successfully merging this pull request may close these issues.

WorldQuery derive doesn't respect visibility
6 participants
@ItsDoot @alice-i-cecile @DJMcNab @infmagic2047 @cart @IceSentry