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

[Merged by Bors] - Basic documentation for Entities, Components and Systems #1578

Closed
wants to merge 9 commits into from
Closed

Conversation

alice-i-cecile
Copy link
Member

These are largely targeted at beginners, as Entity, Component and System are the most obvious terms to search when first getting introduced to Bevy.

@alice-i-cecile alice-i-cecile added C-Docs An addition or correction to our documentation A-ECS Entities, components, systems, and events labels Mar 6, 2021
Co-authored-by: MinerSebas <66798382+MinerSebas@users.noreply.github.com>
Copy link
Contributor

@Ratysz Ratysz left a comment

Choose a reason for hiding this comment

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

I've got a bit carried away during reveiw. You don't have to agree with any of these changes; I'll list my reasons for them anyway.

Component doc reads as subjective: "in a straightforward and efficient fashion", "is very simple", "commonly", "is known as"...

Components are not what stores - they are what is stored.

I understand the desire to use "if and only if", but "X is Trait + OtherTrait" is unambiguous, more popular in Rust docs, and allows for some seriously concise sentences.

It's not necessary to mention Rust. The users do, presumably, know what language they're working with.

I'm partial to dry and impersonal style - this is technical documentation, it should be as to the point as possible. It does have the unfortunate tendency to start all sentences the same way, though.

Trimmed lines to 100 symbols - this is what rustfmt usually targets for code; maintaining that for comments and docs is not required, but I like it for consistency (and it removes the horizontal scrollbar in my IDE).

crates/bevy_ecs/src/component/mod.rs Outdated Show resolved Hide resolved
crates/bevy_ecs/src/entity/mod.rs Outdated Show resolved Hide resolved
crates/bevy_ecs/src/system/system.rs Outdated Show resolved Hide resolved
alice-i-cecile and others added 3 commits March 7, 2021 23:24
Co-authored-by: Alexander Sepity <ratysz@gmail.com>
Co-authored-by: Alexander Sepity <ratysz@gmail.com>
Co-authored-by: Alexander Sepity <ratysz@gmail.com>
@alice-i-cecile
Copy link
Member Author

I liked your changes a lot @Ratysz, and you're likely correct about tone, even if it's a bit drier than is perhaps ideal for certain audiences. We can save a more approachable tone for examples and the book.

Very good idea to include a link to the docs on how to order systems in the System docs.

I had expected rustfmt to trim the doc comments to the appropriate length automatically 😅

crates/bevy_ecs/src/system/system.rs Outdated Show resolved Hide resolved
Co-authored-by: Alexander Sepity <ratysz@gmail.com>
@alice-i-cecile alice-i-cecile 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 Mar 12, 2021
@cart
Copy link
Member

cart commented Mar 12, 2021

Looks good to me!

@cart
Copy link
Member

cart commented Mar 12, 2021

bors r+

bors bot pushed a commit that referenced this pull request Mar 12, 2021
These are largely targeted at beginners, as `Entity`, `Component` and `System` are the most obvious terms to search when first getting introduced to Bevy.
@bors bors bot changed the title Basic documentation for Entities, Components and Systems [Merged by Bors] - Basic documentation for Entities, Components and Systems Mar 12, 2021
@bors bors bot closed this Mar 12, 2021
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.

4 participants