[Merged by Bors] - Added missing details to SystemParam Local documentation.#7106
Closed
iiYese wants to merge 2 commits intobevyengine:mainfrom
Closed
[Merged by Bors] - Added missing details to SystemParam Local documentation.#7106iiYese wants to merge 2 commits intobevyengine:mainfrom
iiYese wants to merge 2 commits intobevyengine:mainfrom
Conversation
…al lifetimes in code.
alice-i-cecile
added
C-Docs
alice-i-cecile
requested changes
Jan 6, 2023
Member
alice-i-cecile
left a comment
alice-i-cecile
left a comment
There was a problem hiding this comment.
Small feedback for clarity, then LGTM.
james7132
approved these changes
Jan 6, 2023
Member
james7132
left a comment
james7132
left a comment
There was a problem hiding this comment.
Approval contingent on the resolution of the currently open comment.
joseph-gio
suggested changes
Jan 6, 2023
| /// If multiple [`SystemParam`]s within the same system each specify the same local type | ||
| /// each will get their own local. | ||
| /// | ||
| /// The supplied lifetime parameter is the [`SystemParam`]s `'s` lifetime. |
Member
There was a problem hiding this comment.
IMO this comment is unnecessary, simply renaming the lifetime to 's is good enough.
Contributor
Author
There was a problem hiding this comment.
While just having 's does express the information it expresses it in a manner that is easy to miss. And given the cost of missing that information
Users can get complicated incorrect suggested fixes if they pass the wrong lifetime
I think it is worth repeating once in a more explicit visible fashion.
Member
There was a problem hiding this comment.
Still not convinced that it's necessary, since that comment isn't adding any information that you can't gather just from glancing at the type.
This isn't a significant complaint though, so I won't try and block on it or anything.
atlv24
approved these changes
Jan 6, 2023
Co-authored-by: Alice Cecile <alice.i.cecile@gmail.com>
iiYese
requested review from
alice-i-cecile and
joseph-gio
and removed request for
alice-i-cecile
joseph-gio
approved these changes
Jan 6, 2023
alice-i-cecile
added
the
S-Ready-For-Final-Review
bors bot
pushed a commit
that referenced
this pull request
Jan 6, 2023
# Objective `SystemParam` `Local`s documentation currently leaves out information that should be documented. - What happens when multiple `SystemParam`s within the same system have the same `Local` type. - What lifetime parameter is expected by `Local`. ## Solution - Added sentences to documentation to communicate this information. - Renamed `Local` lifetimes in code to `'s` where they previously were not. Users can get complicated incorrect suggested fixes if they pass the wrong lifetime. Some instance of the code had `'w` indicating the expected lifetime might not have been known to those that wrote the code either. Co-authored-by: iiYese <83026177+iiYese@users.noreply.github.com>
Contributor
bors
bot
changed the title
james7132
pushed a commit
to james7132/bevy
that referenced
this pull request
Jan 21, 2023
…#7106) # Objective `SystemParam` `Local`s documentation currently leaves out information that should be documented. - What happens when multiple `SystemParam`s within the same system have the same `Local` type. - What lifetime parameter is expected by `Local`. ## Solution - Added sentences to documentation to communicate this information. - Renamed `Local` lifetimes in code to `'s` where they previously were not. Users can get complicated incorrect suggested fixes if they pass the wrong lifetime. Some instance of the code had `'w` indicating the expected lifetime might not have been known to those that wrote the code either. Co-authored-by: iiYese <83026177+iiYese@users.noreply.github.com>
alradish
pushed a commit
to alradish/bevy
that referenced
this pull request
Jan 22, 2023
…#7106) # Objective `SystemParam` `Local`s documentation currently leaves out information that should be documented. - What happens when multiple `SystemParam`s within the same system have the same `Local` type. - What lifetime parameter is expected by `Local`. ## Solution - Added sentences to documentation to communicate this information. - Renamed `Local` lifetimes in code to `'s` where they previously were not. Users can get complicated incorrect suggested fixes if they pass the wrong lifetime. Some instance of the code had `'w` indicating the expected lifetime might not have been known to those that wrote the code either. Co-authored-by: iiYese <83026177+iiYese@users.noreply.github.com>
ItsDoot
pushed a commit
to ItsDoot/bevy
that referenced
this pull request
Feb 1, 2023
…#7106) # Objective `SystemParam` `Local`s documentation currently leaves out information that should be documented. - What happens when multiple `SystemParam`s within the same system have the same `Local` type. - What lifetime parameter is expected by `Local`. ## Solution - Added sentences to documentation to communicate this information. - Renamed `Local` lifetimes in code to `'s` where they previously were not. Users can get complicated incorrect suggested fixes if they pass the wrong lifetime. Some instance of the code had `'w` indicating the expected lifetime might not have been known to those that wrote the code either. Co-authored-by: iiYese <83026177+iiYese@users.noreply.github.com>
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
5 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
Objective
SystemParamLocals documentation currently leaves out information that should be documented.SystemParams within the same system have the sameLocaltype.Local.Solution
Locallifetimes in code to'swhere they previously were not. Users can get complicated incorrect suggested fixes if they pass the wrong lifetime. Some instance of the code had'windicating the expected lifetime might not have been known to those that wrote the code either.