Fix required components documentation ambiguity #16539
Conversation
|
Welcome, new contributor! Please make sure you've read our contributing guide and we look forward to reviewing your pull request shortly ✨ |
|
Sorry about the one word change, didn't really know how to rephrase it to represent how required components actually propogate |
IQuick143
added
C-Docs
|
I don't think that this is quite the right phrasing; it's quite subtle. I'll try and take a crack at this and make a suggestion here, then you tell me if it makes sense to you? :) |
alice-i-cecile
added
S-Needs-Help
alice-i-cecile
changed the title
| @@ -228,7 +228,7 @@ use derive_more::derive::{Display, Error}; | |||
| /// | |||
| /// In general, this shouldn't happen often, but when it does the algorithm is simple and predictable: | |||
| /// 1. Use all of the constructors (including default constructors) directly defined in the spawned component's require list | |||
| /// 2. In the order the requires are defined in `#[require()]`, recursively visit the require list of each of the components in the list (this is a Depth First Search). When a constructor is found, it will only be used if one has not already been found. | |||
| /// 2. In the order the requires are defined in `#[require()]`, recursively visit the require list of each of the components in the list (this is a Breadth First Search). When a constructor is found, it will only be used if one has not already been found. | |||
There was a problem hiding this comment.
| /// 2. In the order the requires are defined in `#[require()]`, recursively visit the require list of each of the components in the list (this is a Breadth First Search). When a constructor is found, it will only be used if one has not already been found. | |
| /// 2. In the order the requires are defined in `#[require()]`, recursively visit the require list of each of the components in the list. When a constructor is found, it will only be used if one has not already been found. | |
| /// | |
| /// This is a Depth First Search, but constructors of a direct requires are evaluated before recursing. |
There was a problem hiding this comment.
That sounds a bit off. Maybe:
/// The algorithm for choosing the constructor from the tree:
/// 1. A constructor from a direct #[requires()], if one exists, is selected with priority.
/// 2. Otherwise, perform a Depth First Search on the tree of requirements and select the first one found.
There was a problem hiding this comment.
I think the main issue with the current wording is that step 1 refers to the "spawned component", which seems to suggest that the step only applies to the component you directly spawn (aka the root of the tree), when it actually applies to every component visited during the DFS. I'd reword it to something roughly like this:
- Starting with the spawned component, recusively visit the require list of each of the components in the list, in the order the requires are defined in
#[require()](this is a Depth First Search).- Whenever a component is visited, use all of the constructors (including default constructors) directly defined in its require list, except for the components whose constructors have already been found.
@BenjaminBrienen's wording is good.
There was a problem hiding this comment.
On my readthrough of it I thought "depth-first" was somewhat misleading, but maybe I'm just misunderstanding the term.
There was a problem hiding this comment.
The implementation is actually a depth-first search, it just collects the constructors whenever it visits a component.
|
I've copied the wording from #16494 (comment) directly for now. I think this is clearer and good enough to merge, but @IllyStable or @benfrankel may have a better suggestion. |
alice-i-cecile
added
S-Needs-Review
Co-authored by: @BenjaminBrienen # Objective Fixes #16494. Closes #16539, which this replaces. Suggestions alone weren't enough, so now we have a new PR! --------- Co-authored-by: Joona Aalto <jondolf.dev@gmail.com>
Co-authored by: @BenjaminBrienen # Objective Fixes #16494. Closes #16539, which this replaces. Suggestions alone weren't enough, so now we have a new PR! --------- Co-authored-by: Joona Aalto <jondolf.dev@gmail.com>
Co-authored by: @BenjaminBrienen # Objective Fixes #16494. Closes #16539, which this replaces. Suggestions alone weren't enough, so now we have a new PR! --------- Co-authored-by: Joona Aalto <jondolf.dev@gmail.com>
Co-authored by: @BenjaminBrienen # Objective Fixes bevyengine#16494. Closes bevyengine#16539, which this replaces. Suggestions alone weren't enough, so now we have a new PR! --------- Co-authored-by: Joona Aalto <jondolf.dev@gmail.com>
Objective
Solution
Testing