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

Replace unwrap calls with expect in bevy_sprite crate #4043

Closed

Conversation

mnmaita
Copy link
Member

@mnmaita mnmaita commented Feb 26, 2022

Objective

Solution

  • Replaced all unwrap calls with expect calls and added comprehensive error messages when these statements panic.

Feel free to review the error messages and provide suggestions on how they could be made clearer.

@github-actions github-actions bot added the S-Needs-Triage This issue needs to be labelled label Feb 26, 2022
@mnmaita mnmaita changed the title Replace unwrap with }}expect in Replace unwrap calls with expect in bevy_sprite crate Feb 26, 2022
@mnmaita
Copy link
Member Author

mnmaita commented Feb 26, 2022

Should I use the following pattern (suggested by clippy) on places where I call a format! macro as the argument for the expect call?

foobar.unwrap_or_else(|_| panic!(/* ... */))

@mnmaita
Copy link
Member Author

mnmaita commented Feb 26, 2022

Also, I was trying to display HandleIds as part of some errors but noticed it's an enum with tuple variants and it doesn't implement Debug, so any suggestion on how to clarify messages on loops or places where a hint on the entity/handle/etc. could be useful is more than welcome.

@james7132 james7132 added A-Rendering Drawing game state to the screen C-Code-Quality A section of code that is hard to understand or change and removed S-Needs-Triage This issue needs to be labelled labels Feb 26, 2022
@james7132
Copy link
Member

Should I use the following pattern (suggested by clippy) on places where I call a format! macro as the argument for the expect call?

foobar.unwrap_or_else(|_| panic!(/* ... */))

Please do, otherwise there will be a String allocated even when there isn't an error.

@ghost ghost mentioned this pull request Feb 26, 2022
31 tasks
@alice-i-cecile
Copy link
Member

Closing per discussion in #3899 <3

@mnmaita mnmaita deleted the mnmaita/bevy-sprite-replace-unwrap branch April 12, 2022 14:12
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-Code-Quality A section of code that is hard to understand or change
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants