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

0.13 migration: Use clippy-approved ReceivedCharacter advice #1045

Merged

Conversation

rparrett
Copy link
Contributor

@rparrett rparrett commented Feb 22, 2024

And other improvements. I went in there to fix the clippy thing and things sort of snowballed.

  • Moved the text to a subsection of the PR that caused the breakage

  • Added some context about why it's not a char and what else might be in there.

  • Added a sentence about a new alternative to ReceivedCharacter that folks may want to use instead.

  • Modified the previous advice so that it is Clippy-approved.

    The previous advice would result in:

    warning: called `.nth(0)` on a `std::iter::Iterator`, when `.next()` is equivalent
       --> src/typing.rs:126:30
        |
    126 |             let Some(char) = event.char.chars().nth(0) else {
        |                              ^^^^^^^^^^^^^^^^^^^^^^^^^ help: try calling `.next()` instead of `.nth(0)`: `event.char.chars().next()`
        |
        = help: for further information visit https://rust-lang.github.io/rust-clippy/master/index.html#iter_nth_zero
        = note: `#[warn(clippy::iter_nth_zero)]` on by default
    

    Clippy suggests .next(), but based on the winit docs, it seems like .last() might be the way to go to me. In the cases where there's more than one char, folks probably want the last one.

Some of this stuff should probably make its way into Bevy's docs or a bevy issue.

@rparrett rparrett added C-Feature A new feature, making something new possible A-Migration Guides labels Feb 22, 2024
@rparrett
Copy link
Contributor Author

Opened docs issue here: bevyengine/bevy#12040

@TrialDragon TrialDragon added the S-Ready-For-Final-Review Ready for a maintainer to consider for merging label Feb 22, 2024
@alice-i-cecile alice-i-cecile added this pull request to the merge queue Feb 22, 2024
Merged via the queue into bevyengine:main with commit 134cbfd Feb 22, 2024
9 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-Migration Guides C-Feature A new feature, making something new possible S-Ready-For-Final-Review Ready for a maintainer to consider for merging
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants