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

document public items, fix doc tests #14

Merged
merged 5 commits into from
Mar 10, 2022
Merged

document public items, fix doc tests #14

merged 5 commits into from
Mar 10, 2022

Conversation

B-Reif
Copy link
Contributor

@B-Reif B-Reif commented Mar 9, 2022

This PR adds a lint to warn for missing docs, adds some documentation for public items that didn't have any, and fixes examples that were failing in rustdoc tests.

Copy link
Owner

@MrGVSV MrGVSV left a comment

Choose a reason for hiding this comment

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

Looks great! This actually came at the perfect time because I'm doing some major rework to the project (transitioning to Bevy's Reflect from the deprecated typetag) and really needed to update the docs for the last patch release of this version. So thank you for this! 😄

Just a few suggestions/changes.

src/components.rs Outdated Show resolved Hide resolved
src/components.rs Outdated Show resolved Hide resolved
src/lib.rs Show resolved Hide resolved
@@ -8,10 +15,11 @@ pub mod prototype;
mod utils;

pub mod prelude {
pub use bevy_proto_derive::*;
//! Includes all public types and the macro to derive [`ProtoComponent`](super::components::ProtoComponent).
Copy link
Owner

Choose a reason for hiding this comment

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

Is it worth suggesting that it's often best used as use bevy_proto::prelude::*? Or is it better to just let the user decide how they want to use it?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I think this might depend on how they're using it, and if they're using all of the bevy_proto types in the same place. I'd just let users decide how to use it.

On a related note I was thinking we could reduce the number of public modules. Specifically, we could hide components and plugin and just pub use components::ProtoComponent and pub use plugin::ProtoPlugin since those mods don't contain anything else anyway.

Copy link
Owner

Choose a reason for hiding this comment

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

Yeah, let's do that.

src/plugin.rs Outdated Show resolved Hide resolved
src/plugin.rs Outdated Show resolved Hide resolved
src/plugin.rs Outdated Show resolved Hide resolved
src/lib.rs Show resolved Hide resolved
src/lib.rs Show resolved Hide resolved
@B-Reif
Copy link
Contributor Author

B-Reif commented Mar 9, 2022

One downside to our current approach is that the blocks in the README aren't tested by cargo test, so it's possible for them to be incorrect / out of date. I'm not sure what the best approach is to solving that.

@MrGVSV
Copy link
Owner

MrGVSV commented Mar 9, 2022

One downside to our current approach is that the blocks in the README aren't tested by cargo test, so it's possible for them to be incorrect / out of date. I'm not sure what the best approach is to solving that.

That's true. Maybe we can do something like: rust-lang/cargo#383 (comment) in lib.rs?

@B-Reif
Copy link
Contributor Author

B-Reif commented Mar 9, 2022

Sure, I'll give it a go

@B-Reif
Copy link
Contributor Author

B-Reif commented Mar 9, 2022

@MrGVSV Two of the doc tests for the README are failing whenever we try to get EntityCommands from ProtoCommands:

error[E0623]: lifetime mismatch
  --> src\lib.rs:397:46
   |
22 |     fn insert_self(&self, proto_commands: &mut ProtoCommands, asset_server: &Res<AssetServer>) {
   |                                                ------------- this type is declared with multiple lifetimes...
23 |         let handle: Handle<Image> = asset_server.load(self.texture_path.as_str());
24 |         let entity_commands = proto_commands.raw_commands();
   |                                              ^^^^^^^^^^^^ ...but data with one lifetime flows into the other here

error: aborting due to previous error

For more information about this error, try `rustc --explain E0623`.
Couldn't compile the test.

I'm not sure how to resolve this. It seems like this error pops up whenever raw_commands is invoked.

@MrGVSV
Copy link
Owner

MrGVSV commented Mar 10, 2022

@MrGVSV Two of the doc tests for the README are failing whenever we try to get EntityCommands from ProtoCommands:

error[E0623]: lifetime mismatch
  --> src\lib.rs:397:46
   |
22 |     fn insert_self(&self, proto_commands: &mut ProtoCommands, asset_server: &Res<AssetServer>) {
   |                                                ------------- this type is declared with multiple lifetimes...
23 |         let handle: Handle<Image> = asset_server.load(self.texture_path.as_str());
24 |         let entity_commands = proto_commands.raw_commands();
   |                                              ^^^^^^^^^^^^ ...but data with one lifetime flows into the other here

error: aborting due to previous error

For more information about this error, try `rustc --explain E0623`.
Couldn't compile the test.

I'm not sure how to resolve this. It seems like this error pops up whenever raw_commands is invoked.

Hm, I wonder if it's because of the lifetime on EntityCommands here:

bevy_proto/src/data.rs

Lines 341 to 343 in 22c84e3

pub fn raw_commands(&'p mut self) -> &'p mut EntityCommands<'w, 's, 'a> {
&mut self.commands
}

Does eliding the 'p lifetime on the references resolve the issue?

If not, we could maybe try making 'p: 'a? Not sure if that would fix it though...

@B-Reif
Copy link
Contributor Author

B-Reif commented Mar 10, 2022

Does eliding the 'p lifetime on the references resolve the issue?

Yes, that did it!

Copy link
Owner

@MrGVSV MrGVSV left a comment

Choose a reason for hiding this comment

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

Looks good! Thank you for these changes!

@MrGVSV MrGVSV merged commit b93a170 into MrGVSV:main Mar 10, 2022
@B-Reif B-Reif deleted the add-docs branch March 10, 2022 15:49
@B-Reif B-Reif mentioned this pull request Mar 12, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants