-
-
Notifications
You must be signed in to change notification settings - Fork 3.9k
Rename get_many()
to many()
, making it consistent with single()
#18615
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
Conversation
All of these were found using ripgrep.
Remove references to `get_many()` and `get_many_mut()`. `single()` and `single_mut()` were also listed twice, so I deleted those as well.
It looks like your PR is a breaking change, but you didn't provide a migration guide. Please review the instructions for writing migration guides, then expand or revise the content in the migration guides directory to reflect your changes. |
You missed some in doc tests :) I'm going to mark this as X-Controversial. I'm in favor of this, but reverted it due to pushback in the linked PR. |
I'm personally on side |
Honestly, I'm convinced. The image I included even supports your argument lol; I must've been tired while writing up this PR. In that case, I think this is a major documentation problem. We currently present |
# Objective - There's been several changes to `Query` for this release cycle, and `Query`'s top-level documentation has gotten slightly out-of-date. - Alternative to #18615. ## Solution - Edit `Query`'s docs for consistency, clarity, and correctness. - Make sure to group `get()` and `get_many()` together instead of `single()` and `get_many()`, to enforce the distinction from #18615 (comment). - Reformat doc tests so they would be readable if extracted into their own file. (Which mainly involves adding more spacing.) - Move link definitions to be nearer where they are used. - Fix the tables so they are up-to-date and correctly escape square brackets `\[ \]`. ## Testing I ran `cargo doc -p bevy_ecs --no-deps` to view the docs and `cargo test -p bevy_ecs --doc` to test the doc comments. ## Reviewing The diff is difficult to read, so I don't recommend _just_ looking at that. Instead, run `cargo doc -p bevy_ecs --no-deps` locally and read through the new version. It should theoretically read smoother with less super-technical jargon. :) ## Follow-up I want to go through some of `Query`'s methods, such as `single()`, `get()`, and `get_many()`, but I'll leave that for another PR. --------- Co-authored-by: Chris Russell <[email protected]>
# Objective - There's been several changes to `Query` for this release cycle, and `Query`'s top-level documentation has gotten slightly out-of-date. - Alternative to #18615. ## Solution - Edit `Query`'s docs for consistency, clarity, and correctness. - Make sure to group `get()` and `get_many()` together instead of `single()` and `get_many()`, to enforce the distinction from #18615 (comment). - Reformat doc tests so they would be readable if extracted into their own file. (Which mainly involves adding more spacing.) - Move link definitions to be nearer where they are used. - Fix the tables so they are up-to-date and correctly escape square brackets `\[ \]`. ## Testing I ran `cargo doc -p bevy_ecs --no-deps` to view the docs and `cargo test -p bevy_ecs --doc` to test the doc comments. ## Reviewing The diff is difficult to read, so I don't recommend _just_ looking at that. Instead, run `cargo doc -p bevy_ecs --no-deps` locally and read through the new version. It should theoretically read smoother with less super-technical jargon. :) ## Follow-up I want to go through some of `Query`'s methods, such as `single()`, `get()`, and `get_many()`, but I'll leave that for another PR. --------- Co-authored-by: Chris Russell <[email protected]>
Objective
There is a pretty glaring inconsistency between
Query::single()
andQuery::get_many()
's naming. Although they both return aResult
, the later was not renamed intomany()
. This inconsistency is problematic because it makes it more difficult to developers to understand what it's doing.Here's an example: across the Rust ecosystem, it's pretty much standard for conversion methods starting with
as_
to be cheap, while methods starting withto_
to be expensive. (Seestr::as_bytes()
vs.Path::to_str()
.) This is documented in the Rust API Guidelines, and is considered "good practice".Bevy also has its own conventions and practices, I even wrote
bevy_lint
to help enforce these. Before better error handling was introduced, we had the following convention for panicking methods:get_foo()
returned aResult
, whilefoo()
panicked. There are 17 examples of this inWorld
,Query
, andQueryState
alone.With Bevy 0.16, this convention is being changed. The panicking methods of old are no longer a thing, having been deprecated or replaced.
Query::single()
now returns aResult
instead of panicking, with the oldQuery::get_single()
being deprecated as an alias. This treatment wasn't given toQuery::many()
, however, because "folks weren't sold on the namemany
(get_many is clearer, and this is rare), and that PR is much more complex" (#18183).Yes, the name
get_many()
is clearer that it returns aResult
, but so isget_single()
! Why should one method be given the ✨special✨ treatment just because it is more useful? There are too many parallels betweensingle()
andmany()
to cause such a large rift like this. For example, look at the Accessing query items table:Even the wording is the same! The only differences are making "item" and "entity" plural.
So here's my point: developers will see the similarity between
single()
andget_many()
, but will be confused why their naming is different when the do the exact same thing.For additional context, see #18120 and #18183.
Solution
many()
andmany_mut()
.get_many_*()
methods (includingget_many()
andget_many_mut()
) tomany_*()
.get_many()
andget_many_mut()
that are simply deprecated aliases tomany()
andmany_mut()
. (This is not needed forget_many_unique_mut()
and friends because they are a new addition to 0.16.)get_many()
and friends.Testing
I relied heavily on
cargo check
,cargo test
, and ripgrep to verify my changes were thorough. CI should catch anything I missed, but feel free to do you own checks!Migration Guide
This migration guide should be merged with the original one that make
Query::single()
return aResult
, and will replace #18183's migration guide.Query::many()
andQuery::many_mut()
(and theQueryState
versions) now return aResult
instead of panicking.Query::get_many()
andQuery::get_many_mut()
are now deprecated.