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] - Add readme as docs to relevant crates. #2575

Closed
wants to merge 11 commits into from

Conversation

Hoidigan
Copy link
Contributor

@Hoidigan Hoidigan commented Jul 31, 2021

Fixes #2566
Fixes #3005

There are only READMEs in the 4 crates here (with the exception of bevy itself).
Those 4 crates are ecs, reflect, tasks, and transform.
These should each now include their respective README files.

@github-actions github-actions bot added the S-Needs-Triage This issue needs to be labelled label Jul 31, 2021
@alice-i-cecile alice-i-cecile added C-Docs An addition or correction to our documentation and removed S-Needs-Triage This issue needs to be labelled labels Jul 31, 2021
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.

Awesome, thank you!

@alice-i-cecile
Copy link
Member

bors try

bors bot added a commit that referenced this pull request Jul 31, 2021
@bors
Copy link
Contributor

bors bot commented Jul 31, 2021

try

Build failed:

@Nilirad
Copy link
Contributor

Nilirad commented Aug 1, 2021

I tried to do the same on my #2365 PR, but the examples in README.md break the doc test suite 😕

@alice-i-cecile
Copy link
Member

@mockersf, any ideas on how to work around this CI issue?

@Nilirad
Copy link
Contributor

Nilirad commented Aug 1, 2021

It's not only a CI issue though. Running a simple command like

cargo test -p bevy_ecs --doc

would fail.

@Hoidigan
Copy link
Contributor Author

Hoidigan commented Aug 1, 2021

We could ignore the doc tests? Put " ```rust ignore " for the start of the blocks instead of " ```rust "
Either that or go through and put the lines that get ignored but make the doc test work. I'm not sure if github would still show them or not, though.
(https://doc.rust-lang.org/rustdoc/documentation-tests.html#hiding-portions-of-the-example)

@alice-i-cecile
Copy link
Member

I'm not sure if github would still show them or not, though.

Yeah, I think Github is likely to just show the extra lines of code :( Let's try ignoring them; it's no worse than the current situation where they're never compiled.

@Nilirad
Copy link
Contributor

Nilirad commented Aug 1, 2021

I don't know if ignoring will retain the code highlighting on GitHub renders...

@Hoidigan
Copy link
Contributor Author

Hoidigan commented Aug 1, 2021

Hmmm, yeah I don't know either.
I'll try a few and see.

@alice-i-cecile
Copy link
Member

https://github.com/bevyengine/bevy/blob/9f096ecb830b15943bef87d520e1c696de6868e7/crates/bevy_ecs/README.md works!

@Hoidigan
Copy link
Contributor Author

Hoidigan commented Aug 1, 2021

I'll go through and grab the rest then 😄

Ignore the doc tests in the readme, this allows "cargo test --doc
-p *" to run without errors
@Hoidigan
Copy link
Contributor Author

Hoidigan commented Aug 1, 2021

Alrighty they should be ignored now, cargo test --doc -p * worked fine for me.

Nilirad added a commit to Nilirad/bevy that referenced this pull request Aug 7, 2021
@mockersf
Copy link
Member

mockersf commented Aug 9, 2021

I'm not a fan of ignoring the failing code examples as a big plus of including the README is to test those examples. Would it be hard to fix them?

@Hoidigan
Copy link
Contributor Author

Hoidigan commented Aug 9, 2021

The issue wasn't that they would be hard to fix (I don't think so but I'd need to check again once I can) but that if we fixed them we'd either clutter up examples in the docs and GitHub, or we'd do the hide lines thing for docs.rs but it would show the lines still on GitHub.

It's up a little bit in the conversation, we talked about it.

@mockersf
Copy link
Member

mockersf commented Aug 9, 2021

It's up a little bit in the conversation, we talked about it.

Yup, but it depends: is allowing the code blocks to work just adding 2 lines of imports? I think that's worth it. Is it adding 20 lines of setup? Ok that can be ignored, or maybe the example can be reworked?

@Hoidigan
Copy link
Contributor Author

Hoidigan commented Aug 9, 2021

I'm not entirely sure and I'm not in a position to check right now, but in the next day or two I should be able to see. (Maybe tonight, I don't know.)

@Hoidigan
Copy link
Contributor Author

Hoidigan commented Aug 10, 2021

Alrighty, so I went through and made the examples work.
There were some things I was a little iffy about, namely declaring Position and Velocity for a lot of examples in bevy_ecs, but that fits in two lines. Most of them were solved by that and importing the prelude.
I did take bevy_core as a dev dependency to bring in the time resource.
Bevy_ecs does now have all working examples and none ignored. Most of them are only 3 or 4 lines longer. (and there was a few actual errors in them too, so that's fixed now.)

bevy_reflect has all of the setup at the top example. I could go through and copy and paste it into every other example but that's the 20 lines of setup we wanted to avoid.
Would it be possible (or wanted) to create a small crate inside of reflect and then have a dev-dependency on that for some setup function and imports?

Tasks and transform had no examples so that's easy enough.

Copy link
Member

@NiklasEi NiklasEi left a comment

Choose a reason for hiding this comment

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

Good call to include the Readmes, thanks!
Can we try to keep the Readme examples of bevy_ecs free of other bevy crates? I think of this Readme as showing examples of how to use bevy_ecs standalone in e.g. other engines or completely different contexts than gamedev.

@@ -68,11 +87,17 @@ fn print_position(query: Query<(Entity, &Position)>) {
Apps often require unique resources, such as asset collections, renderers, audio servers, time, etc. Bevy ECS makes this pattern a first class citizen. `Resource` is a special kind of component that does not belong to any entity. Instead, it is identified uniquely by its type:

```rust
use bevy_ecs::prelude::*;

// Bevy does provide its own more advanced Time as a resource, but we
Copy link
Member

Choose a reason for hiding this comment

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

Bevys' Time is part of the core crate. I would prefer keeping this readme purely for bevy_ecs, so I would remove this comment here.

@@ -174,6 +211,9 @@ fn system(query: Query<&Position, Added<Velocity>>) {
Resources also expose change state:

```rust
use bevy_core::Time;
Copy link
Member

Choose a reason for hiding this comment

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

See comment above. Can we get rid of the bevy_core dependency?

@NiklasEi NiklasEi added S-Ready-For-Final-Review This PR has been approved by the community. It's ready for a maintainer to consider merging it and removed S-Needs-Review labels Oct 8, 2021
@NiklasEi
Copy link
Member

@alice-i-cecile could we add this to 0.6? It would be nice to have the readmes in the official documentation of the next release. And the change is pretty uncontroversial I would say.

@NiklasEi
Copy link
Member

@Hoidigan could you add #3005 as getting resolved by this PR?

@Hoidigan
Copy link
Contributor Author

Hoidigan commented Nov 23, 2021

Fixes #3005

(that is how you do that, right? Just put that here?)

@NiklasEi
Copy link
Member

Fixes #3005

(that is how you do that, right? Just put that here?)

I think it needs to be in the commit message. So in this case in the PR description.

@DJMcNab DJMcNab mentioned this pull request Dec 8, 2021
#[derive(Default)]
struct Time {
seconds: f32,
Copy link
Member

Choose a reason for hiding this comment

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

I object to using a tuple struct here. The named field is way better at documenting intent. Tuple structs rarely have a place in public apis imo.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds good to me 👍

@cart
Copy link
Member

cart commented Dec 18, 2021

One small comment. Everything else looks good to me!

@cart
Copy link
Member

cart commented Dec 18, 2021

bors r+

bors bot pushed a commit that referenced this pull request Dec 18, 2021
Fixes #2566
Fixes #3005 

There are only READMEs in the 4 crates here (with the exception of bevy itself).
Those 4 crates are ecs, reflect, tasks, and transform.
These should each now include their respective README files.

Co-authored-by: Hoidigan <57080125+Hoidigan@users.noreply.github.com>
Co-authored-by: Daniel Nelsen <57080125+Hoidigan@users.noreply.github.com>
@bors bors bot changed the title Add readme as docs to relevant crates. [Merged by Bors] - Add readme as docs to relevant crates. Dec 18, 2021
@bors bors bot closed this Dec 18, 2021
@Hoidigan Hoidigan deleted the crate-level-docs branch December 18, 2021 23:43
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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
7 participants