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] - Add DrawFunctionsInternals::id() #6745

Closed
wants to merge 3 commits into from

Conversation

IceSentry
Copy link
Contributor

@IceSentry IceSentry commented Nov 24, 2022

Objective

  • Every usage of DrawFunctionsInternals::get_id() was followed by a .unwrap(). which just adds boilerplate.

Solution

  • Introduce a fallible version of DrawFunctionsInternals::get_id() and use it where possible.
  • I also took the opportunity to improve the error message a little in the case where it fails.

Changelog

  • Added DrawFunctionsInternals::id()

@IceSentry IceSentry added the A-Rendering Drawing game state to the screen label Nov 24, 2022
@james7132 james7132 added C-Code-Quality A section of code that is hard to understand or change M-Needs-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide labels Nov 24, 2022
Copy link
Member

@james7132 james7132 left a comment

Choose a reason for hiding this comment

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

Changes make sense, definitely makes the code here cleaner. Unfortunately this is a public interface despite the type clearly being marked as Internal in it's name. Can you please add short migration guide blurb?

@james7132 james7132 added this to the 0.10 milestone Nov 24, 2022
@IceSentry
Copy link
Contributor Author

I didn't add any migration guide because it's just adding a new function and therefore shouldn't be breaking. Does adding new functions count as breaking?

@IceSentry
Copy link
Contributor Author

There isn't really anything to migrate here.

@james7132 james7132 removed the M-Needs-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide label Nov 24, 2022
@james7132 james7132 modified the milestones: 0.10, 0.9.1 Nov 24, 2022
@james7132
Copy link
Member

Sorry, must have missed that. Thought it was replacing get_id for whatever reason.

@IceSentry
Copy link
Contributor Author

IceSentry commented Nov 24, 2022

No worries, technically it could replace it, but might as well leave it in there. Could always be useful in the future.

@alice-i-cecile alice-i-cecile modified the milestones: 0.9.1, 0.10 Nov 25, 2022
@alice-i-cecile alice-i-cecile 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 Nov 25, 2022
@alice-i-cecile
Copy link
Member

bors r+

bors bot pushed a commit that referenced this pull request Nov 28, 2022
# Objective

- Every usage of `DrawFunctionsInternals::get_id()` was followed by a `.unwrap()`. which just adds boilerplate.

## Solution

- Introduce a fallible version of `DrawFunctionsInternals::get_id()` and use it where possible.
- I also took the opportunity to improve the error message a little in the case where it fails.

---

## Changelog

- Added `DrawFunctionsInternals::id()`
@bors
Copy link
Contributor

bors bot commented Nov 28, 2022

Build failed (retrying...):

bors bot pushed a commit that referenced this pull request Nov 28, 2022
# Objective

- Every usage of `DrawFunctionsInternals::get_id()` was followed by a `.unwrap()`. which just adds boilerplate.

## Solution

- Introduce a fallible version of `DrawFunctionsInternals::get_id()` and use it where possible.
- I also took the opportunity to improve the error message a little in the case where it fails.

---

## Changelog

- Added `DrawFunctionsInternals::id()`
@bors bors bot changed the title Add DrawFunctionsInternals::id() [Merged by Bors] - Add DrawFunctionsInternals::id() Nov 28, 2022
@bors bors bot closed this Nov 28, 2022
@IceSentry IceSentry deleted the draw_function_id branch November 28, 2022 21:50
taiyoungjang pushed a commit to taiyoungjang/bevy that referenced this pull request Dec 15, 2022
# Objective

- Every usage of `DrawFunctionsInternals::get_id()` was followed by a `.unwrap()`. which just adds boilerplate.

## Solution

- Introduce a fallible version of `DrawFunctionsInternals::get_id()` and use it where possible.
- I also took the opportunity to improve the error message a little in the case where it fails.

---

## Changelog

- Added `DrawFunctionsInternals::id()`
alradish pushed a commit to alradish/bevy that referenced this pull request Jan 22, 2023
# Objective

- Every usage of `DrawFunctionsInternals::get_id()` was followed by a `.unwrap()`. which just adds boilerplate.

## Solution

- Introduce a fallible version of `DrawFunctionsInternals::get_id()` and use it where possible.
- I also took the opportunity to improve the error message a little in the case where it fails.

---

## Changelog

- Added `DrawFunctionsInternals::id()`
ItsDoot pushed a commit to ItsDoot/bevy that referenced this pull request Feb 1, 2023
# Objective

- Every usage of `DrawFunctionsInternals::get_id()` was followed by a `.unwrap()`. which just adds boilerplate.

## Solution

- Introduce a fallible version of `DrawFunctionsInternals::get_id()` and use it where possible.
- I also took the opportunity to improve the error message a little in the case where it fails.

---

## Changelog

- Added `DrawFunctionsInternals::id()`
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 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