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

Fix intra-doc links and make CI test them #14076

Merged
merged 5 commits into from
Jul 11, 2024
Merged

Fix intra-doc links and make CI test them #14076

merged 5 commits into from
Jul 11, 2024

Conversation

SkiFire13
Copy link
Contributor

Objective

  • Bevy currently has lot of invalid intra-doc links, let's fix them!
  • Also make CI test them, to avoid future regressions.
  • Helps with dead link tracking #1983 (but doesn't fix it, as there could still be explicit links to docs.rs that are broken)

Solution

  • Make cargo r -p ci -- doc-check check fail on warnings (could also be changed to just some specific lints)
  • Manually fix all the warnings (note that in some cases it was unclear to me what the fix should have been, I'll try to highlight them in a self-review)

SkiFire13
SkiFire13 commented Jun 30, 2024
View reviewed changes
Copy link
Contributor Author

@SkiFire13 SkiFire13 left a comment

Choose a reason for hiding this comment

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

Sorry if the comments are a bit out of order, at some point Github wanted me to refresh the page and I lost some comments so I had to re-add them later on.

crates/bevy_color/src/color.rs Outdated Show resolved Hide resolved
crates/bevy_core_pipeline/src/core_3d/main_opaque_pass_3d_node.rs
Comment on lines -20 to +21
/// A [`bevy_render::render_graph::Node`] that runs the [`Opaque3d`]
/// [`BinnedRenderPhase`] and [`AlphaMask3d`]
/// [`bevy_render::render_phase::SortedRenderPhase`]s.
/// A [`bevy_render::render_graph::Node`] that runs the [`Opaque3d`] and [`AlphaMask3d`]
/// [`ViewBinnedRenderPhases`]s.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I'm not familiar with the render pipelines, but from the code below it seems ViewBinnedRenderPhases was being used and not BinnedRenderPhase or SortedRenderPhase

crates/bevy_core_pipeline/src/core_3d/main_transmissive_pass_3d_node.rs
/// [`SortedRenderPhase`].
/// [`ViewSortedRenderPhases`].
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Similarly here.

crates/bevy_core_pipeline/src/core_3d/main_transparent_pass_3d_node.rs
/// [`SortedRenderPhase`].
/// [`ViewSortedRenderPhases`].
Copy link
Contributor Author

Choose a reason for hiding this comment

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

And here.

crates/bevy_ecs/src/world/mod.rs
/// Calls both [`World::flush_entities`] and [`World::flush_commands`].
/// Flushes queued entities and calls [`World::flush_commands`].
Copy link
Contributor Author

Choose a reason for hiding this comment

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

World::flush_entities is private so I removed the reference.

crates/bevy_winit/src/lib.rs
/// The [`winit::event_loop::EventLoopProxy`] with the specific [`winit::event::Event::UserEvent`] used in the [`winit_runner`].
/// A re-export of [`winit::event_loop::EventLoopProxy`].
Copy link
Contributor Author

Choose a reason for hiding this comment

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

The old description didn't seem to make much sense, and also it referenced winit_runner which is private.

crates/bevy_asset/src/loader.rs
Comment on lines -291 to +290
/// An error that occurs when attempting to call [`LoadContext::load_direct`]
/// An error that occurs when attempting to call [`DirectNestedLoader::load`](crate::DirectNestedLoader::load)
Copy link
Contributor Author

Choose a reason for hiding this comment

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

I didn't find any LoadContext::load_direct, the closest thing where this is used seemed to be DirectNestedLoader::load but I'm not sure it's correct.

crates/bevy_ecs/src/entity/map_entities.rs
Comment on lines -195 to +196
/// [`Entity`] while reserving extra generations via [`crate::entity::Entities::reserve_generations`]. Because this
/// renders the [`SceneEntityMapper`] unable to safely allocate any more references, this method takes ownership of
/// `self` in order to render it unusable.
/// [`Entity`] while reserving extra generations. Because this makes the [`SceneEntityMapper`] unable to
/// safely allocate any more references, this method takes ownership of `self` in order to render it unusable.
Copy link
Contributor Author

Choose a reason for hiding this comment

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

Entities::reserve_generations is private so I removed the reference.

crates/bevy_ecs/src/observer/runner.rs Show resolved Hide resolved
crates/bevy_ecs/src/observer/runner.rs Show resolved Hide resolved
@janhohenheim janhohenheim added C-Docs An addition or correction to our documentation A-Cross-Cutting Impacts the entire engine X-Uncontroversial This work is generally agreed upon S-Needs-Review Needs reviewer attention (from anyone!) to move forward D-Straightforward Simple bug fixes and API improvements, docs, test and examples labels Jun 30, 2024
@BD103 BD103 self-requested a review June 30, 2024 17:47
BD103
BD103 approved these changes Jun 30, 2024
View reviewed changes
Copy link
Member

@BD103 BD103 left a comment

Choose a reason for hiding this comment

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

From a quick pass this looks good, though I didn't have the time to make a thorough check.

@alice-i-cecile alice-i-cecile added S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it and removed S-Needs-Review Needs reviewer attention (from anyone!) to move forward labels Jul 11, 2024
@alice-i-cecile
Copy link
Member

Tell me when this is green? This is definitely an improvement over the state of things and we should get it merged before it rots.

@SkiFire13
Copy link
Contributor Author

Seems like doc errors are more frequent than expected seeing how 3 PRs introduced them in the span of just 2 weeks. I should have fixed the new errors, hopefully CI is green is ~10 minutes. I would like to merge this as soon as possible to prevent further errors which will block this PR even more.

@alice-i-cecile alice-i-cecile added this pull request to the merge queue Jul 11, 2024
Merged via the queue into bevyengine:main with commit d708036 Jul 11, 2024
27 checks passed
@SkiFire13 SkiFire13 deleted the fix-doc-links branch July 11, 2024 13:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@NthTensor NthTensor NthTensor left review comments

@BD103 BD103 BD103 approved these changes

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

Assignees
No one assigned
Labels
A-Cross-Cutting Impacts the entire engine C-Docs An addition or correction to our documentation D-Straightforward Simple bug fixes and API improvements, docs, test and examples S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it X-Uncontroversial This work is generally agreed upon
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@SkiFire13 @alice-i-cecile @NthTensor @BD103 @janhohenheim