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

Improvement of AssetServer::load documentation to help find a way to load from file with hash in filename #13272

Open
wants to merge 1 commit into
base: main
Choose a base branch
from

Conversation

bugsweeper
Copy link
Contributor

@bugsweeper bugsweeper commented May 7, 2024

Objective

Solution

  • Improved documentation for AssetServer::load.

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.

I like this feature but I feel pretty strongly that these should be constructor methods rather than From impls. They're easier to discover and much nicer to document: even just the method and argument names make things a lot less error prone.

@alice-i-cecile alice-i-cecile added A-Assets Load files from disk to use for things like images, models, and sounds C-Usability A simple quality-of-life change that makes Bevy easier to use D-Straightforward Simple bug fixes and API improvements, docs, test and examples labels May 7, 2024
@bugsweeper
Copy link
Contributor Author

bugsweeper commented May 8, 2024

@alice-i-cecile I used From<(Path, label)> because AssetServer::load uses Into<AssetPath> in arguments. If you prefer constructor there is a way (more complex, but exists) for such thing:
asset_server.load(AssetPath::from_path(Path::new("A#.ron")).with_label("Scene0"));
I proposed simplier way:
asset_server.load((Path::new("A#.ron"), "Scene0"));
I agree that documenting should be done, I could add this way of loading into AssetServer::load doc-comment.
If this is bad idea we can close this PR and parent issue, because constructor way already exists.

@bugsweeper
Copy link
Contributor Author

May be we should improve just AssetServer::load doc-comment with explicit path and label example

@alice-i-cecile
Copy link
Member

May be we should improve just AssetServer::load doc-comment with explicit path and label example

Great idea :) That's definitely my preference.

@bugsweeper bugsweeper force-pushed the assetpath-from-path-and-label branch 2 times, most recently from 31c6743 to d3a92a4 Compare May 8, 2024 20:20
@bugsweeper bugsweeper force-pushed the assetpath-from-path-and-label branch from d3a92a4 to dead8ea Compare May 8, 2024 20:24
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.

Looks great. Just remember to update the title and PR description!

@alice-i-cecile alice-i-cecile added C-Docs An addition or correction to our documentation X-Uncontroversial This work is generally agreed upon S-Needs-Review Needs reviewer attention (from anyone!) to move forward and removed C-Usability A simple quality-of-life change that makes Bevy easier to use labels May 10, 2024
@bugsweeper bugsweeper changed the title From implementation for AssetPath from Path and label Improvement of AssetServer::load to help find a way to load from file with hash in filename May 10, 2024
@bugsweeper bugsweeper changed the title Improvement of AssetServer::load to help find a way to load from file with hash in filename Improvement of AssetServer::load documentation to help find a way to load from file with hash in filename May 10, 2024
@bugsweeper
Copy link
Contributor Author

bugsweeper commented May 17, 2024

@alice-i-cecile Is there anything else I could do to move forward?

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 D-Straightforward Simple bug fixes and API improvements, docs, test and examples S-Needs-Review Needs reviewer attention (from anyone!) to move forward X-Uncontroversial This work is generally agreed upon
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Asset Server incorrectly parsing files with # in their name
2 participants