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

Order roughly by impact #5

Merged
merged 1 commit into from
Nov 4, 2022

Conversation

rparrett
Copy link

@rparrett rparrett commented Nov 4, 2022

Diff looks super weird so it's a bit hard to be 1000% confident that I didn't mangle this, but I did glance at the rendered output and verify that the same number of lines/headers are present.

There are many sections that seemed very unlikely to break user code or that I didn't really understand so it was a bit difficult.

Many sections are not actually breaking and should be deleted and I'll follow up with individual suggestions later.

Scene/reflect stuff maybe should be higher, but those sections are huge and they are at least grouped togetherish now.

@IceSentry
Copy link
Owner

oof, yeah, the diff is something else, I'll pull it locally and do quick run over before merging.

@IceSentry
Copy link
Owner

Is there any kind of heuristic for the ordering or is it purely by feeling? Just want to know if the generator could output something in a better order in the future. Maybe weight it based on the amount of reacts?

@rparrett
Copy link
Author

rparrett commented Nov 4, 2022

My only thought is "number of examples impacted" but that it would be a bit of a lift.

This effort was purely by feeling after migrating a couple personal projects and a couple ecosystem crates.

@IceSentry
Copy link
Owner

This seems good to me

Notes for after the merge
Move impl bundle for components closer to spawn take bundle
Move the time scaling closer to bool argument on timer just because it's time related
Move the removal of watch_for_changes closer to new plugin settings just to keep similar changes close together
Remove bevy_pbr:/bevy_scene and other similar prefix for consistency

In the future, I think I'll generate the migration guide based on Area tag. It would probably be easier to follow.

@IceSentry IceSentry merged commit 5edb226 into IceSentry:migration-guide-0.9 Nov 4, 2022
@rparrett
Copy link
Author

rparrett commented Nov 4, 2022

Those tweaks all make sense to me.

@IceSentry
Copy link
Owner

I'll commit them in a few minutes

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants