[Merged by Bors] - Borrow instead of consuming in EventReader::clear
#6851
Conversation
alice-i-cecile
added
A-ECS
|
Not necessarily against this change but this was done on purpose because the reader is useless after calling |
Could you add more details on how that's unusable? Once You'll have to fix other things (see CI failures), I think the fact that it was consumed was useful for something but can't remember what from the top of my head. |
james7132
added
the
M-Needs-Migration-Guide
|
This still changes a public facing API which makes it a breaking change. @mvlabat do you mind writing up a short changelog and migration guide? |
Consuming the event reader means it is not possible to do something like this this: if my_condition {
event_reader.clear();
}
for event in event_reader.iter() {
...
}The |
|
I can see how it could be annoying, hence the 'Not necessarily against this change'. |
|
I think this is a reasonable change. I don't think we need a migration guide as this is a relaxing of constraints. Before, calling clear() would result in EventReader being dropped / no longer available. This means no code after it could use it. After the change, that code will behave identically: clear will still be the last usage of EventReader and it will be dropped. Functionally maybe there is some contrived case where that difference matters, but I can't come up with one. |
|
I added a changelog entry though |
|
bors r+ |
The PR fixes the interface of `EventReader::clear`. Currently, the method consumes the reader, which makes it unusable. ## Changelog - `EventReader::clear` now takes a mutable reference instead of consuming the event reader.
|
Build failed: |
|
Welp the build found a non-trivial breaking change, so I was wrong this does need a migration guide. I'll fix that now. |
|
bors r+ |
The PR fixes the interface of `EventReader::clear`. Currently, the method consumes the reader, which makes it unusable.
## Changelog
- `EventReader::clear` now takes a mutable reference instead of consuming the event reader.
## Migration Guide
`EventReader::clear` now takes a mutable reference instead of consuming the event reader. This means that `clear` now needs explicit mutable access to the reader variable, which previously could have been omitted in some cases:
```rust
// Old (0.9)
fn clear_events(reader: EventReader<SomeEvent>) {
reader.clear();
}
// New (0.10)
fn clear_events(mut reader: EventReader<SomeEvent>) {
reader.clear();
}
```
Co-authored-by: Carter Anderson <[email protected]>
|
Build failed: |
|
bors r+ |
The PR fixes the interface of `EventReader::clear`. Currently, the method consumes the reader, which makes it unusable.
## Changelog
- `EventReader::clear` now takes a mutable reference instead of consuming the event reader.
## Migration Guide
`EventReader::clear` now takes a mutable reference instead of consuming the event reader. This means that `clear` now needs explicit mutable access to the reader variable, which previously could have been omitted in some cases:
```rust
// Old (0.9)
fn clear_events(reader: EventReader<SomeEvent>) {
reader.clear();
}
// New (0.10)
fn clear_events(mut reader: EventReader<SomeEvent>) {
reader.clear();
}
```
Co-authored-by: Carter Anderson <[email protected]>
bors
bot
changed the title
EventReader::clearEventReader::clear
The PR fixes the interface of `EventReader::clear`. Currently, the method consumes the reader, which makes it unusable.
## Changelog
- `EventReader::clear` now takes a mutable reference instead of consuming the event reader.
## Migration Guide
`EventReader::clear` now takes a mutable reference instead of consuming the event reader. This means that `clear` now needs explicit mutable access to the reader variable, which previously could have been omitted in some cases:
```rust
// Old (0.9)
fn clear_events(reader: EventReader<SomeEvent>) {
reader.clear();
}
// New (0.10)
fn clear_events(mut reader: EventReader<SomeEvent>) {
reader.clear();
}
```
Co-authored-by: Carter Anderson <[email protected]>
|
Oh, that was merged sooner than I was able to reply! Thanks! :D Just to add to @JoJoJet's comment, consuming the event reader also makes it more difficult to use inside structs that derive @cart thank you for adding the change log and migration guide. I thought that the patch was quite trivial and didn't imply breaking changes, so I omitted them, but you were right to note that it requires specifying mutability in the arguments. |
The PR fixes the interface of `EventReader::clear`. Currently, the method consumes the reader, which makes it unusable.
## Changelog
- `EventReader::clear` now takes a mutable reference instead of consuming the event reader.
## Migration Guide
`EventReader::clear` now takes a mutable reference instead of consuming the event reader. This means that `clear` now needs explicit mutable access to the reader variable, which previously could have been omitted in some cases:
```rust
// Old (0.9)
fn clear_events(reader: EventReader<SomeEvent>) {
reader.clear();
}
// New (0.10)
fn clear_events(mut reader: EventReader<SomeEvent>) {
reader.clear();
}
```
Co-authored-by: Carter Anderson <[email protected]>
The PR fixes the interface of `EventReader::clear`. Currently, the method consumes the reader, which makes it unusable.
## Changelog
- `EventReader::clear` now takes a mutable reference instead of consuming the event reader.
## Migration Guide
`EventReader::clear` now takes a mutable reference instead of consuming the event reader. This means that `clear` now needs explicit mutable access to the reader variable, which previously could have been omitted in some cases:
```rust
// Old (0.9)
fn clear_events(reader: EventReader<SomeEvent>) {
reader.clear();
}
// New (0.10)
fn clear_events(mut reader: EventReader<SomeEvent>) {
reader.clear();
}
```
Co-authored-by: Carter Anderson <[email protected]>
The PR fixes the interface of
EventReader::clear. Currently, the method consumes the reader, which makes it unusable.Changelog
EventReader::clearnow takes a mutable reference instead of consuming the event reader.Migration Guide
EventReader::clearnow takes a mutable reference instead of consuming the event reader. This means thatclearnow needs explicit mutable access to the reader variable, which previously could have been omitted in some cases: