-
-
Notifications
You must be signed in to change notification settings - Fork 3.9k
Document required components with a marker trait #18169
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
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -207,6 +207,28 @@ pub fn derive_component(input: TokenStream) -> TokenStream { | |
let struct_name = &ast.ident; | ||
let (impl_generics, type_generics, where_clause) = &ast.generics.split_for_impl(); | ||
|
||
let required_component_impls = attrs.requires.as_ref().map(|r| { | ||
let impls = r | ||
.iter() | ||
.map(|r| { | ||
let path = &r.path; | ||
let insertion_info = match &r.func { | ||
Some(RequireFunc::Closure(closure)) => format!("`{}`", closure.body.to_token_stream()), | ||
Some(RequireFunc::Path(path)) => format!("[`{}`].", path.to_token_stream()), | ||
None => "the [default](Default::default) value.".to_string(), | ||
}; | ||
quote! { | ||
/// If not already present, the required component will be inserted using | ||
#[doc = #insertion_info] | ||
impl #impl_generics #bevy_ecs_path::component::document_required_components::Require<#path> for #struct_name #type_generics #where_clause {} | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I'm not convinced that this is meaningfully "better" than the current "required list in Component docs":
From my perspective the only real win here is that the require list shows up in the sidebar, but that is also a downside, as a long list takes up significant space and obscures other "real" trait impls. And it doesn't necessarily meaningfully improve the prominence of the require list. The sidebar is often littered with other things (ex: I have to scroll to see the list for Node). Edit: The backlinks are kind of nice, but the fact that There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Additionally, our docs are just generally already very "noisy" and this adds even more to the pile. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think these are valid concerns, and I don't think this technique can be amended to satisfy them. Thanks for providing the feedback! I'll close this out |
||
} | ||
}); | ||
|
||
quote! { | ||
#(#impls)* | ||
} | ||
}); | ||
|
||
let required_component_docs = attrs.requires.map(|r| { | ||
let paths = r | ||
.iter() | ||
|
@@ -270,6 +292,8 @@ pub fn derive_component(input: TokenStream) -> TokenStream { | |
#relationship | ||
|
||
#relationship_target | ||
|
||
#required_component_impls | ||
}) | ||
} | ||
|
||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -455,6 +455,24 @@ pub trait Component: Send + Sync + 'static { | |
fn visit_entities_mut(_this: &mut Self, _f: impl FnMut(&mut Entity)) {} | ||
} | ||
|
||
// doc(hidden) module makes it clear that usage of this trait outside of `bevy_ecs` is unsupported. | ||
#[doc(hidden)] | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. For me, this removes all Require listings from the docs, which defeats the purpose of this feature :) There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. Ah whoops, should be tested first! I assumed keeping the trait public would keep it in the documentation. |
||
pub mod document_required_components { | ||
use super::Component; | ||
|
||
/// Indicates this [`Component`] requires another [`Component`] `C`. | ||
/// | ||
/// **This trait does not register required components on its own.** | ||
/// | ||
/// This trait is similar to [`Eq`] in the sense that it is up to the implementer to ensure `C` | ||
/// is appropriately registered as a required component. | ||
/// | ||
/// You should never manually implement this trait, but it is the implementor's responsibility | ||
/// to ensure the implementation of [`Component::register_required_components`] matches any and | ||
/// all implementations of [`Require`] on this type. | ||
pub trait Require<C: Component>: Component {} | ||
} | ||
|
||
mod private { | ||
pub trait Seal {} | ||
} | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add #[cfg(doc)] to the generated impl/trait?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sadly not, I tried but
cargo doc
doesn't propagatedoc
to dependencies or macros in a way that worked reliably.There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Yep, that's because of rust-lang/cargo#8811. For docs.rs and dev-doc.bevyengine.org you could set the flags in
Cargo.toml
/docs.yaml
.