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.

Sign up for GitHub

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

Jump to bottom

[Merged by Bors] - Rework manual event iterator so we can actually name the type #5735

Closed
wants to merge 13 commits into from
Closed

[Merged by Bors] - Rework manual event iterator so we can actually name the type #5735

wants to merge 13 commits into from

Conversation

Aceeri
Copy link
Member

@Aceeri Aceeri commented Aug 18, 2022 •
edited
Loading

Objective

Solution

  • Create a custom Iterator type.

Changelog

  • oldest_id and get_event convenience methods added to Events<T>.

@IceSentry IceSentry added the A-ECS Entities, components, systems, and events label Aug 19, 2022
cart
cart reviewed Aug 30, 2022
View reviewed changes
crates/bevy_ecs/src/event.rs Outdated Show resolved Hide resolved
@cart cart mentioned this pull request Aug 30, 2022
Closed
@alice-i-cecile alice-i-cecile added the C-Usability A targeted quality-of-life change that makes Bevy easier to use label Sep 2, 2022
JoJoJet
JoJoJet approved these changes Dec 22, 2022
View reviewed changes
Copy link
Member

@JoJoJet JoJoJet left a comment

Choose a reason for hiding this comment

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

Everything here looks good to me.

In the future, I think we should consider renaming .iter(). It always feels wrong and surprising that events.iter() mutates events, and it's especially surprising that for _ in &events does not work. Borrowing from std::io::Read, I think a potentially better name for that method would be .read().

@Aceeri
Copy link
Member Author

Aceeri commented Dec 22, 2022

I think it somewhat makes sense in that events is basically a saved iterator, we aren't actually mutating Events<E> we are mutating the ManualEventReader, but shrug.

@JoJoJet
Copy link
Member

JoJoJet commented Dec 22, 2022 •
edited
Loading

Mutating self makes perfect sense -- I just don't think the method that does it should be called iter. Anyway, that's not what this PR is about, I just thought I'd mention it.

@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 Dec 22, 2022
@Aceeri Aceeri mentioned this pull request Dec 22, 2022
Closed
alice-i-cecile
alice-i-cecile approved these changes Dec 24, 2022
View reviewed changes
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.

Can you add the little utility methods (get_events and friends) to the change log too? They're quite useful, but easy to overlook.

bors r+

bors bot pushed a commit that referenced this pull request Dec 25, 2022
@Aceeri
Rework manual event iterator so we can actually name the type (#5735)
486ce72 
# Objective
- Be able to name the type that `ManualEventReader::iter/iter_with_id` returns and `EventReader::iter/iter_with_id` by proxy.
  Currently for the purpose of #5719

## Solution
- Create a custom `Iterator` type.
@bors
Copy link
Contributor

bors bot commented Dec 25, 2022

Build failed (retrying...):

bors bot pushed a commit that referenced this pull request Dec 25, 2022
@Aceeri
Rework manual event iterator so we can actually name the type (#5735)
317051b 
# Objective
- Be able to name the type that `ManualEventReader::iter/iter_with_id` returns and `EventReader::iter/iter_with_id` by proxy.
  Currently for the purpose of #5719

## Solution
- Create a custom `Iterator` type.
@bors
Copy link
Contributor

bors bot commented Dec 25, 2022

Build failed (retrying...):

bors bot pushed a commit that referenced this pull request Dec 25, 2022
@Aceeri
Rework manual event iterator so we can actually name the type (#5735)
9717204 
# Objective
- Be able to name the type that `ManualEventReader::iter/iter_with_id` returns and `EventReader::iter/iter_with_id` by proxy.
  Currently for the purpose of #5719

## Solution
- Create a custom `Iterator` type.
@bors
Copy link
Contributor

bors bot commented Dec 25, 2022

Pull request successfully merged into main.

Build succeeded:

@bors bors bot changed the title Rework manual event iterator so we can actually name the type [Merged by Bors] - Rework manual event iterator so we can actually name the type Dec 25, 2022
@bors bors bot closed this Dec 25, 2022
alradish pushed a commit to alradish/bevy that referenced this pull request Jan 22, 2023
@Aceeri @alradish
Rework manual event iterator so we can actually name the type (bevyen…
0e0d6ac 
…gine#5735)

# Objective
- Be able to name the type that `ManualEventReader::iter/iter_with_id` returns and `EventReader::iter/iter_with_id` by proxy.
  Currently for the purpose of bevyengine#5719

## Solution
- Create a custom `Iterator` type.
ItsDoot pushed a commit to ItsDoot/bevy that referenced this pull request Feb 1, 2023
@Aceeri @ItsDoot
Rework manual event iterator so we can actually name the type (bevyen…
fac90e8 
…gine#5735)

# Objective
- Be able to name the type that `ManualEventReader::iter/iter_with_id` returns and `EventReader::iter/iter_with_id` by proxy.
  Currently for the purpose of bevyengine#5719

## Solution
- Create a custom `Iterator` type.
@urben1680 urben1680 mentioned this pull request Oct 3, 2024
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@cart cart cart left review comments

@afonsolage afonsolage afonsolage approved these changes

@alice-i-cecile alice-i-cecile alice-i-cecile approved these changes

@JoJoJet JoJoJet JoJoJet approved these changes

Assignees
No one assigned
Labels
A-ECS Entities, components, systems, and events C-Usability A targeted quality-of-life change that makes Bevy easier to use 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
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
6 participants
@Aceeri @JoJoJet @afonsolage @cart @alice-i-cecile @IceSentry