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

rename 'w and 's to 'world and 'state #6958

Closed
wants to merge 4 commits into from
Closed

rename 'w and 's to 'world and 'state #6958

wants to merge 4 commits into from

Conversation

LamequeFernandes
Copy link

@LamequeFernandes LamequeFernandes commented Dec 15, 2022

Co-Authored-by: Peh099 pedrohenriquec099@gmail.com

Objective

Rename 'w and 's to 'world and 'state in SystemParam. As described in the first topic of issue #6953.

Solution

Rename arguments in documentation.


Changelog

Renamed 'w and 's to 'world and 'state in documentation about SystemParam .

Co-Authored-by: Peh099 <pedrohenriquec099@gmail.com>
@LamequeFernandes
Copy link
Author

We wondered if it was also necessary to rename the 'w and 's of the functions that were outside the comments in the documentation.

@MrGVSV
Copy link
Member

MrGVSV commented Dec 15, 2022

I could be wrong, but isn't 'w and 's required when using the SystemParam derive? If so, this change might be confusing. Maybe you could just give a call out to the the fact that they're for 'world and 'state (e.g. "...the 'w lifetime (otherwise known as the world lifetime)...")


Edit: Yeah looks like it. Here are the results from check-doc:

error: invalid lifetime name: expected `'w` or `'s`
        'w -- refers to data stored in the World.
        's -- refers to data stored in the SystemParam's state.'
 --> src/system/system_param.rs:1528:21
  |
7 | struct GenericParam<'world,'state, T: SystemParam + 'static> {
  |                     ^^^^^^

@MrGVSV MrGVSV added C-Docs An addition or correction to our documentation A-ECS Entities, components, systems, and events labels Dec 15, 2022
@nicopap nicopap added X-Controversial There is active debate or serious implications around merging this PR and removed X-Controversial There is active debate or serious implications around merging this PR labels Dec 15, 2022
@nicopap
Copy link
Contributor

nicopap commented Dec 15, 2022

See also #6953

@inodentry
Copy link
Contributor

Bikeshed: should it be 'system instead of 'state?

@alice-i-cecile
Copy link
Member

Bikeshed: should it be 'system instead of 'state?

No, this isn't only useful in systems.

Copy link
Member

@alice-i-cecile alice-i-cecile left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@LamequeFernandes it looks like you need to update the SystemParam macro for this to pass CI.

@alice-i-cecile
Copy link
Member

Nice: can you resolve conflicts? Either merge main into this branch or rebase this PR.

…arams-system-param

Co-authored-by: Peh099 <pedrohenriquec099@gmail.com>
@LamequeFernandes
Copy link
Author

Nice: can you resolve conflicts? Either merge main into this branch or rebase this PR.

Yes, I can, I'll do it with @Peh099 right now :)

@alice-i-cecile
Copy link
Member

Rebase looks good, but looks like there's a couple places to touch up still :)

@LamequeFernandes LamequeFernandes closed this by deleting the head repository Mar 23, 2023
@CGMossa
Copy link
Contributor

CGMossa commented Mar 23, 2023

Was it done elsewhere?

@alice-i-cecile
Copy link
Member

No; this still needs to be redone.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-ECS Entities, components, systems, and events C-Docs An addition or correction to our documentation
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants