Skip to content

Conversation

@SUPERCILEX
Copy link
Contributor

@SUPERCILEX SUPERCILEX commented Jan 25, 2022

No description provided.

@github-actions github-actions bot added the S-Needs-Triage This issue needs to be labelled label Jan 25, 2022
@alice-i-cecile alice-i-cecile added C-Code-Quality A section of code that is hard to understand or change C-Examples An addition or correction to our examples and removed S-Needs-Triage This issue needs to be labelled labels Jan 25, 2022
@alice-i-cecile
Copy link
Member

I generally really like these changes! They clean things up a bit, and are well-motivated. In real code, I would probably prefer the for_each pattern for perf, but the for loop is easier to read for beginners (and Cart has asked us to use that style in examples).

@SUPERCILEX
Copy link
Contributor Author

@alice-i-cecile Is there anything I need to do after the approval?

@alice-i-cecile
Copy link
Member

alice-i-cecile commented Jan 27, 2022

@alice-i-cecile Is there anything I need to do after the approval?

Nope, you're all good. If you'd like, you can ask around and wrangle up another approval from the community. Once that's in place, I can merge this in.

(I have merge rights for docs and examples).

@SUPERCILEX
Copy link
Contributor Author

Makes sense, thanks!

@rparrett
Copy link
Contributor

While we're at it, it would be a bit more idiomatic for that timer to be a resource rather than a component.

@SUPERCILEX
Copy link
Contributor Author

@rparrett Like this? 52d57fa

@rparrett
Copy link
Contributor

@rparrett Like this? 52d57fa

That looks a lot nicer!

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.

I like the further changes! A bit iffy on the clippy ignore, but I agree that it's a clippy bug.

# Conflicts:
#	examples/2d/contributors.rs
Signed-off-by: Alex Saveau <[email protected]>
@SUPERCILEX
Copy link
Contributor Author

@alice-i-cecile @rparrett Looks like this should finally be ready for a merge. Any outstanding concerns?

@rparrett
Copy link
Contributor

rparrett commented Apr 8, 2022

Just a nit, but a few other examples (plugin, font_atlas_debug) use a naming scheme that would look like

struct SelectionState {
    timer: Timer,
    has_triggered: bool,
}

When combining a timer with some other state in a resource.

Signed-off-by: Alex Saveau <[email protected]>
@SUPERCILEX
Copy link
Contributor Author

Sounds good, done!

Copy link
Contributor

@rparrett rparrett left a comment

Choose a reason for hiding this comment

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

This changeset seems like an improvement to me. I tested with an artificially shortened contributor list to ensure that the wrapping still works.

@alice-i-cecile
Copy link
Member

bors r+

@bors bors bot changed the title Slight perf improvements and tidy for contributors example [Merged by Bors] - Slight perf improvements and tidy for contributors example Apr 8, 2022
@bors bors bot closed this Apr 8, 2022
@SUPERCILEX
Copy link
Contributor Author

Woohoo, thanks!

@SUPERCILEX SUPERCILEX deleted the tmp2 branch May 17, 2022 04:21
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

C-Code-Quality A section of code that is hard to understand or change C-Examples An addition or correction to our examples

Projects

None yet

Development

Successfully merging this pull request may close these issues.

3 participants