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

Clarifying Commands' purpose #10837

Merged

Conversation

xNapha
Copy link
Contributor

@xNapha xNapha commented Dec 2, 2023

Objective

As described in Issue #10805 I have changed "impactful changes" to "structural changes"

Solution

Updated the text "impactful" to "structural"

Copy link
Contributor

github-actions bot commented Dec 2, 2023

Welcome, new contributor!

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

@ickk ickk added C-Docs An addition or correction to our documentation A-ECS Entities, components, systems, and events labels Dec 2, 2023
@Nilirad Nilirad added the S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it label Dec 2, 2023
@james7132 james7132 added this pull request to the merge queue Dec 2, 2023
Merged via the queue into bevyengine:main with commit 24c6a7d Dec 2, 2023
26 checks passed
@rlidwka
Copy link
Contributor

rlidwka commented Dec 2, 2023

"Structural changes" is not a well-known term. In the entire Bevy docs, this is only mentioned once:

This is only required if a manual method (such as [Self::get_manual]) is being called, and it only needs to
be called if the world has been structurally mutated (i.e. added/removed a component or resource). Users using
non-manual methods such as [QueryState::get] do not need to call this as it will be automatically called for them.

This change is technically correct I suppose, but without that additional explanation added, this change would be meaningless for most people.

If you start using "structural changes" in docs, you should define somewhere what it means exactly (not necessarily in Commands, just somewhere).

@xNapha
Copy link
Contributor Author

xNapha commented Dec 2, 2023

😮 Ahhh okay that makes sense, im willing to add additional information or provide an example of what it means to "structurally change" something.

Since this is the first time I am contributing to an open-source project and im not too familiar with the whole process, should this additional change, you have suggested, be dicussed with the original issuer? or should this be created as another issue?

@rlidwka
Copy link
Contributor

rlidwka commented Dec 3, 2023

should this additional change, you have suggested, be dicussed with the original issuer? or should this be created as another issue?

Nah, don't bother, it's merged already, and it is in my opinion isn't worth a commit.

You can consider it as a note in case someone decides to make similar changes elsewhere.

@xNapha
Copy link
Contributor Author

xNapha commented Dec 3, 2023

oooh okay, ill definitely keep this in mind in the future! 😄

@xNapha xNapha deleted the Issue/#10805-clarify-commands-purpose branch December 3, 2023 21:51
@doonv doonv mentioned this pull request Jan 6, 2024
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 S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

6 participants