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

Fix doc comment on AssetActionMinimal #11105

Merged
merged 3 commits into from Mar 11, 2024

Conversation

Davier
Copy link
Contributor

@Davier Davier commented Dec 27, 2023

Objective

The doc comment on AssetActionMinimal links to itself instead of AssetAction

Copy link
Contributor

@Nilirad Nilirad left a comment

Choose a reason for hiding this comment

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

I think the second link can be entirely removed. We don't need to link twice in the same sentence. We can go even one step further and reword the sentence to avoid mentioning the name twice.

@Nilirad Nilirad added C-Docs An addition or correction to our documentation A-Assets Load files from disk to use for things like images, models, and sounds labels Dec 27, 2023
@Davier
Copy link
Contributor Author

Davier commented Dec 27, 2023

I think the second link can be entirely removed. We don't need to link twice in the same sentence. We can go even one step further and reword the sentence to avoid mentioning the name twice.

This repetition is present on AssetMetaMinimal, AssetActionMinimal, ProcessedInfoMinimal and arguably AssetMetaDyn, so it could have been intentional (maybe for clarity).
I personally don't care either way, but if you do want that change, I should update all of them. I'll think of an alternative phrasing, feel free to make suggestions.

@Nilirad
Copy link
Contributor

Nilirad commented Dec 31, 2023

Since the scope of this PR is quite small, I think you can re-scope it to remove all the repetitions. It may be reworded to something like:

/// Minimal version of [`X`].
///
/// Speeds up or enables serialization where the non-minimal version of the item is not needed.

Also keep in mind that when splitting in two sentences, repetition is less of a problem. In any case, there shouldn't be a double link.

@BD103
Copy link
Member

BD103 commented Feb 1, 2024

This PR can be closed. The comment has been updated sometime around Asset V2. AssetMetaMinimal

@rparrett
Copy link
Contributor

rparrett commented Feb 1, 2024

This PR can be closed. The comment has been updated sometime around Asset V2.

This PR is post-assets-v2 and is about AssetActionMinimal rather than AssetMetaMinimal.

Copy link
Contributor

@rparrett rparrett left a comment

Choose a reason for hiding this comment

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

This is an improvement as-is.

@BD103
Copy link
Member

BD103 commented Feb 1, 2024

This PR is post-assets-v2 and is about AssetActionMinimal rather than AssetMetaMinimal.

Whoops! My bad 😅

@TrialDragon TrialDragon 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 5, 2024
@mockersf
Copy link
Member

mockersf commented Mar 11, 2024

rebasing to get new required ci jobs... but forgot that I should have wait a few more minutes 🤦

@alice-i-cecile alice-i-cecile added this pull request to the merge queue Mar 11, 2024
Merged via the queue into bevyengine:main with commit bc1073e Mar 11, 2024
25 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Assets Load files from disk to use for things like images, models, and sounds 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.

None yet

7 participants