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

[Merged by Bors] - Document PipelineCache and related types #5600

Closed
wants to merge 7 commits into from

Conversation

djeedai
Copy link
Contributor

@djeedai djeedai commented Aug 6, 2022

Objective

Document PipelineCache and a few other related types.

Solution

Add documenting comments to PipelineCache and a few other related
types in the same file.

Add documenting comments to `PipelineCache` and a few other related
types in the same file.
@alice-i-cecile alice-i-cecile added C-Docs An addition or correction to our documentation A-Rendering Drawing game state to the screen labels Aug 7, 2022
@alice-i-cecile
Copy link
Member

Two small concerns, then this LGTM.

@djeedai
Copy link
Contributor Author

djeedai commented Aug 7, 2022

@alice-i-cecile for final (?) review after all fixed, thanks!

@djeedai
Copy link
Contributor Author

djeedai commented Aug 7, 2022

Tiny rant : changes like #3979 that introduced a large part of this code should not have been allowed to merge without a single comment on the entire file. I don't know if there's a policy now, but if there's none I'd like to suggest raising the bar a bit and strongly suggesting (if not requiring) at least a minimal documenting of new features.

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.

@bevyengine/rendering-team can I get a review from one of y'all? I'm happy with this from a docs perspective but not fully confident about the domain specific content.

Copy link
Member

@cart cart left a comment

Choose a reason for hiding this comment

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

Generally looks good. Just one comment

crates/bevy_render/src/render_resource/pipeline_cache.rs Outdated Show resolved Hide resolved
bors bot pushed a commit that referenced this pull request Aug 8, 2022
# Objective

`ShaderData` is marked as public, but is an internal type only used by one other
internal type, so it should be made private.

## Solution

`ShaderData` is only used in `ShaderCache`, and the latter is private,
so there is no need to make the former public. This change removes the
`pub` keyword from `ShaderData`, hidding it as the implementation detail
it is.

Split from #5600
@djeedai djeedai 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 Sep 2, 2022
@alice-i-cecile
Copy link
Member

bors r+

bors bot pushed a commit that referenced this pull request Sep 2, 2022
# Objective

Document `PipelineCache` and a few other related types.

## Solution

Add documenting comments to `PipelineCache` and a few other related
types in the same file.
@bors bors bot changed the title Document PipelineCache and related types [Merged by Bors] - Document PipelineCache and related types Sep 2, 2022
@bors bors bot closed this Sep 2, 2022
maccesch pushed a commit to Synphonyte/bevy that referenced this pull request Sep 28, 2022
# Objective

`ShaderData` is marked as public, but is an internal type only used by one other
internal type, so it should be made private.

## Solution

`ShaderData` is only used in `ShaderCache`, and the latter is private,
so there is no need to make the former public. This change removes the
`pub` keyword from `ShaderData`, hidding it as the implementation detail
it is.

Split from bevyengine#5600
james7132 pushed a commit to james7132/bevy that referenced this pull request Oct 28, 2022
# Objective

`ShaderData` is marked as public, but is an internal type only used by one other
internal type, so it should be made private.

## Solution

`ShaderData` is only used in `ShaderCache`, and the latter is private,
so there is no need to make the former public. This change removes the
`pub` keyword from `ShaderData`, hidding it as the implementation detail
it is.

Split from bevyengine#5600
james7132 pushed a commit to james7132/bevy that referenced this pull request Oct 28, 2022
# Objective

Document `PipelineCache` and a few other related types.

## Solution

Add documenting comments to `PipelineCache` and a few other related
types in the same file.
ItsDoot pushed a commit to ItsDoot/bevy that referenced this pull request Feb 1, 2023
# Objective

`ShaderData` is marked as public, but is an internal type only used by one other
internal type, so it should be made private.

## Solution

`ShaderData` is only used in `ShaderCache`, and the latter is private,
so there is no need to make the former public. This change removes the
`pub` keyword from `ShaderData`, hidding it as the implementation detail
it is.

Split from bevyengine#5600
ItsDoot pushed a commit to ItsDoot/bevy that referenced this pull request Feb 1, 2023
# Objective

Document `PipelineCache` and a few other related types.

## Solution

Add documenting comments to `PipelineCache` and a few other related
types in the same file.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Rendering Drawing game state to the screen 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.

3 participants