Skip to content

[Merged by Bors] - Fixed docs for derive(WorldQuery).#5283

Closed
CGMossa wants to merge 8 commits intobevyengine:mainfrom
CGMossa:docs_derive_worldquery
Closed

[Merged by Bors] - Fixed docs for derive(WorldQuery).#5283
CGMossa wants to merge 8 commits intobevyengine:mainfrom
CGMossa:docs_derive_worldquery

Conversation

@CGMossa
Copy link
Contributor

@CGMossa CGMossa commented Jul 11, 2022 •
edited
Loading

For derive(WorldQuery), there are three structs generated, Item, Fetch and State.
These inherit the visibility of the derived structure, thus #![warn(missing_docs)] would
warn about missing documentation for these structures.

  • I'd like some advice on what to write here, as I personally don't really understand Fetch nor State.

@alice-i-cecile alice-i-cecile added C-Docs An addition or correction to our documentation A-ECS Entities, components, systems, and events labels Jul 11, 2022
alice-i-cecile
alice-i-cecile requested changes Jul 11, 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.

Feedback:

  1. You probably want to add ` inside the brackets to make sure the link formats correctly.
  2. Could definitely use some better base docs; I'll leave some basic explanations.
  3. I'd leave doc(hidden) on there.

alice-i-cecile
alice-i-cecile reviewed Jul 11, 2022
View reviewed changes
alice-i-cecile
alice-i-cecile reviewed Jul 11, 2022
View reviewed changes
alice-i-cecile
alice-i-cecile reviewed Jul 11, 2022
View reviewed changes
@Nilirad
Copy link
Contributor

Nilirad commented Jul 11, 2022

I thought that #[automatically_derived] items wouldn't trigger a warning.

@alice-i-cecile
Copy link
Member

alice-i-cecile commented Jul 11, 2022

Yeah, I'm surprised to hear that this is still causing you problems. I think these are worth adding regardless however.

@alice-i-cecile
Copy link
Member

alice-i-cecile commented Jul 13, 2022

@CGMossa this needs to be formatted before it can be merged :)

@CGMossa
Copy link
Contributor Author

CGMossa commented Jul 13, 2022

Done. Sorry :( I cannot believe I didn't notice.

@alice-i-cecile
Copy link
Member

alice-i-cecile commented Jul 13, 2022

@BoxyUwU @james7132 are my docs for the generated structs basically fine?

@BoxyUwU
Copy link
Member

BoxyUwU commented Jul 13, 2022

havign docs on a doc(hidden) thing seems weird to me. the docs on the item type I think would be wrong for the ReadOnly version as it would link to the wrong worldquery?

@CGMossa
Copy link
Contributor Author

CGMossa commented Jul 14, 2022

I added a suggestion; I don't know if it improves things or not :(

@CGMossa
Copy link
Contributor Author

CGMossa commented Jul 20, 2022

@BoxyUwU have you seen what I've suggested we change it to?
Hopefully this works for you.

@BoxyUwU
Copy link
Member

BoxyUwU commented Jul 20, 2022

I'm not sure what you mean by "suggested"- anyway the docs look fine to me but it seems like no commits were added? Whoops I must have misread the docs last time sorry

@CGMossa
Copy link
Contributor Author

CGMossa commented Jul 21, 2022 •
edited
Loading

Do you not see this Screenshot_20220721-071908_GitHub.jpg

@alice-i-cecile
Copy link
Member

alice-i-cecile commented Jul 21, 2022

Nope; you need to finish your review. That's what "pending" means.

@CGMossa
Copy link
Contributor Author

CGMossa commented Jul 21, 2022 •
edited
Loading

I've messed this up. I was waiting for Boxy, but it turns out that I was the one creating confusion. Here's what I suggested. The thing is that the link to [`ReadOnlyWorldQuery`] doesn't seem to work.

@CGMossa CGMossa mentioned this pull request Nov 1, 2022
Closed
@bzm3r
Copy link
Contributor

bzm3r commented Nov 1, 2022

@CGMossa I think Alice is fine with it as is. Could you resolve conflicts?

@CGMossa
Copy link
Contributor Author

CGMossa commented Nov 1, 2022

Ah.. I have to rebase.. I'll see if I get to it tomorrow.

@CGMossa
Added a generic doc sentence for all three
4e440d4 
derive structures generated.. through
`derive(WorldQuery)`.
@CGMossa
Added suggestions by @alice-i-cecile.
be4e9bc
@CGMossa
Added a generic doc sentence for all three
38a51cf 
derive structures generated.. through
`derive(WorldQuery)`.

Added suggestions by @alice-i-cecile.

Hiding doc for
state types.

`cargo fmt`.
@CGMossa CGMossa force-pushed the docs_derive_worldquery branch from 82bc53d to be4e9bc Compare November 1, 2022 22:01
@CGMossa
Copy link
Contributor Author

CGMossa commented Nov 1, 2022

Please @bzm3r to look at the changes once more, and see if something was lost the messy rebase I did on this.

@CGMossa
Copy link
Contributor Author

CGMossa commented Nov 1, 2022

Don't merge this please; Testing if it serves it purpose shows that something is missing:

(base) PS C:\Users\angus\Documents\GitHub\bevy_fresh> cargo check --examples
    Blocking waiting for file lock on build directory
    Checking bevy v0.9.0-dev (C:\Users\angus\Documents\GitHub\bevy_fresh)
error: missing documentation for a struct
  --> examples/ecs/custom_query_param.rs:41:1
   |
41 | pub struct ReadOnlyCustomQuery<T: Component + Debug, P: Component + Debug> {
   | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^
   |
note: the lint level is defined here
  --> examples/ecs/custom_query_param.rs:14:11
   |
14 | #![forbid(missing_docs)]
   |           ^^^^^^^^^^^^

error: could not compile `bevy` due to previous error

@CGMossa
Read-only queries has a special
16b932e 
struct created that needs documentation.
@CGMossa
Copy link
Contributor Author

CGMossa commented Nov 1, 2022

This is now fixed.

@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 Nov 1, 2022
@alice-i-cecile
Copy link
Member

alice-i-cecile commented Nov 1, 2022

bors r+

bors bot pushed a commit that referenced this pull request Nov 1, 2022
@CGMossa
Fixed docs for derive(WorldQuery). (#5283)
1f22d54 
For `derive(WorldQuery)`, there are three structs generated, `Item`, `Fetch` and `State`. 
These inherit the visibility of the derived structure, thus `#![warn(missing_docs)]` would
warn about missing documentation for these structures.

- [ ] I'd like some advice on what to write here, as I personally don't really understand `Fetch` nor `State`.
@bors
Copy link
Contributor

bors bot commented Nov 1, 2022

Pull request successfully merged into main.

Build succeeded:

@bors bors bot changed the title Fixed docs for derive(WorldQuery). [Merged by Bors] - Fixed docs for derive(WorldQuery). Nov 1, 2022
@bors bors bot closed this Nov 1, 2022
ItsDoot pushed a commit to ItsDoot/bevy that referenced this pull request Feb 1, 2023
@CGMossa @ItsDoot
Fixed docs for derive(WorldQuery). (bevyengine#5283)
4ec0a25 
For `derive(WorldQuery)`, there are three structs generated, `Item`, `Fetch` and `State`. 
These inherit the visibility of the derived structure, thus `#![warn(missing_docs)]` would
warn about missing documentation for these structures.

- [ ] I'd like some advice on what to write here, as I personally don't really understand `Fetch` nor `State`.
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

+1 more reviewer

@bzm3r bzm3r bzm3r approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

No one assigned

Labels

A-ECS Entities, components, systems, and events C-Docs An addition or correction to our documentation 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

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@CGMossa @Nilirad @alice-i-cecile @BoxyUwU @bzm3r