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

Add missing documentation to bevy_time #9428

Merged
merged 14 commits into from
Aug 15, 2023

Conversation

JoJoJet
Copy link
Member

@JoJoJet JoJoJet commented Aug 12, 2023

Objective

  • Add documentation to undocumented items in bevy_time.
  • Add a warning for undocumented items.

@JoJoJet JoJoJet added C-Docs An addition or correction to our documentation A-Time Involves time keeping and reporting labels Aug 12, 2023
@alice-i-cecile
Copy link
Member

Progress towards #3492

@Selene-Amanita
Copy link
Member

Selene-Amanita commented Aug 12, 2023

Not introduced by this PR but can be fixed by it, wrong backticks in https://docs.rs/bevy/0.11.0/bevy/time/fixed_timestep/struct.FixedTime.html#method.expend:
[`Err(FixedUpdateError`)]
(should probably link to FixedUpdateError not Err too)

(the doc of expend is not clear at all either, which I guess is fine since it's not supposed to be used by a lot of people, only people who make tests or make their own runner, but maybe the doc should say that then?)

Copy link
Member

@Selene-Amanita Selene-Amanita left a comment

Choose a reason for hiding this comment

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

The doc on this page is great: https://docs.rs/bevy_time/0.11.0/bevy_time/fixed_timestep/index.html but FixedTime and run_fixed_update_schedule don't link back to it. Since FixedUpdate links to run_fixed_update_schedule, I think it would be great to link back to the central doc on fixed time, people (me at least) don't always think to check module documentation.

crates/bevy_time/src/lib.rs Outdated Show resolved Hide resolved
crates/bevy_time/src/lib.rs Show resolved Hide resolved
crates/bevy_time/src/lib.rs Outdated Show resolved Hide resolved
@JoJoJet
Copy link
Member Author

JoJoJet commented Aug 13, 2023

Thank you for the review. I believe I have addressed your points.

Copy link
Member

@Selene-Amanita Selene-Amanita left a comment

Choose a reason for hiding this comment

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

Besides a small typo looks good to me!

crates/bevy_time/src/lib.rs Outdated Show resolved Hide resolved
@Selene-Amanita Selene-Amanita 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 Aug 13, 2023
@alice-i-cecile alice-i-cecile added this pull request to the merge queue Aug 14, 2023
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Aug 14, 2023
@alice-i-cecile alice-i-cecile added this pull request to the merge queue Aug 14, 2023
@github-merge-queue github-merge-queue bot removed this pull request from the merge queue due to failed status checks Aug 14, 2023
@GuillaumeGomez
Copy link
Contributor

To help you with missing docs, you can add #![deny(missing_docs)]. :)

@alice-i-cecile
Copy link
Member

@GuillaumeGomez a warn equivalent was added in lib.rs :) Like we discussed in the linked issue, warn is less intrusive for development, but still blocks CI.

@GuillaumeGomez
Copy link
Contributor

Nice! There is also rustdoc::missing_doc_code_examples which could be helpful but I think it's nightly only. Bevy docs is really lacking examples. ^^'

@alice-i-cecile alice-i-cecile added this pull request to the merge queue Aug 15, 2023
Merged via the queue into bevyengine:main with commit f99dcad Aug 15, 2023
21 checks passed
@JoJoJet JoJoJet deleted the document-time branch August 17, 2023 00:02
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Time Involves time keeping and reporting 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
Development

Successfully merging this pull request may close these issues.

4 participants