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

Renamed add_plugins to add_plugin_group to be more clear #7643

Closed
wants to merge 5 commits into from

Conversation

CmPons
Copy link

@CmPons CmPons commented Feb 12, 2023

This is my first PR so please let me know if I'm doing something incorrectly. This touches quite a few files, but it's largely just renaming the add_plugins function.

That being said, would we favor deprecating usage of add_plugins somehow (such as emitting a warning if possible?) instead of just removing it outright? Let me know if that's more desirable and I can change this PR.

@alice-i-cecile FYI!

Objective

Solution

  • Rename add_plugins to add_plugin_group.

Migration Guide

-Users will have to change all usages of add_plugins to add_plugin_group in order to compile without warnings.

@github-actions
Copy link
Contributor

Welcome, new contributor!

Please make sure you've read our contributing guide and we look forward to reviewing your pull request shortly ✨

@alice-i-cecile alice-i-cecile added A-ECS Entities, components, systems, and events C-Usability A targeted quality-of-life change that makes Bevy easier to use M-Needs-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide A-App Bevy apps and plugins and removed A-ECS Entities, components, systems, and events labels Feb 12, 2023
@alice-i-cecile
Copy link
Member

alice-i-cecile commented Feb 12, 2023

This looks correct :)

That said, for such a simple but noisy change, it would be nice to add a deprecation warning.

To do so:

  1. Add a new App::add_plugins method, which just calls add_plugin_group.
  2. Add a #[deprecated] annotation to that method.
  3. Make sure to set the since and note field for that annotation.

See https://doc.rust-lang.org/reference/attributes/diagnostics.html#the-deprecated-attribute for more details.

@alice-i-cecile
Copy link
Member

Yep, we'll need to be careful to check that the version number of the depreciation matches when we actually ship this.

@alice-i-cecile alice-i-cecile requested a review from cart February 12, 2023 22:56
@alice-i-cecile
Copy link
Member

alice-i-cecile commented Feb 12, 2023

@cart I've requested your review because I know you're going to have opinions on this change.

My arguments are:

  1. This is a very common typo for beginners, and results in some serious friction since the error message returned is not very helpful.
  2. PluginGroups are underdeveloped and underused, and I think being more explicit about what's going on will help educate users about them.

@cart
Copy link
Member

cart commented Feb 13, 2023

Hmmmm here are my counter arguments:

  1. It is shorter. Not always the right thing to bias toward. But people generally like it when names aren't super long.
  2. add_plugins(DefaultPlugins) already exists in pretty much every Bevy app and tutorial. Happy to make breaking changes in the interest of making things better. But this largely feels like a lateral move to me.
  3. It follows a consistent naming pattern: add_X(DefaultX), where X is "plugins".
  4. It has parity with our system apis. add_system(a), add_systems((a, b)). The add_plugin(X) and add_plugins(YPlugins) line up well. Note that SystemConfigs is very similar conceptually to PluginGroup. We might even want to make this work: add_plugins((X, Y)).

@alice-i-cecile
Copy link
Member

Mostly agreed on 1 and 2. 3 I think applies just as well to PluginGroup?

4 is the most compelling to me.

If we had the ability to customize error messages to improve this footgun I'd happily close this out. Actually, let me see if we could improve these diagnostics upstream in Rust...

@alice-i-cecile alice-i-cecile added the X-Controversial There is active debate or serious implications around merging this PR label Feb 14, 2023
@cart
Copy link
Member

cart commented Feb 14, 2023

3 I think applies just as well to PluginGroup?

Not quite as well. We'd need to rename DefaultPlugins to DefaultPluginGroup for equal amounts of parity.

@janhohenheim
Copy link
Member

Closing in tandem with #7631

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-App Bevy apps and plugins C-Usability A targeted quality-of-life change that makes Bevy easier to use M-Needs-Migration-Guide A breaking change to Bevy's public API that needs to be noted in a migration guide X-Controversial There is active debate or serious implications around merging this PR
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants