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

Use #[doc(fake_variadic)] to improve docs readability #14703

Merged
merged 5 commits into from
Aug 12, 2024

Conversation

bash
Copy link
Contributor

@bash bash commented Aug 11, 2024

Objective

Solution

This PR modifies the existing all_tuples! macro to optionally accept a #[doc(fake_variadic)] attribute in its input. If the attribute is present, each invocation of the impl macro gets the correct attributes (i.e. the first impl receives #[doc(fake_variadic)] while the other impls are hidden using #[doc(hidden)].
Impls for the empty tuple (unit type) are left untouched (that's what the standard library and serde do).

To work around rust-lang/cargo#8811 and to get impls on re-exports to correctly show up as variadic, --cfg docsrs_dep is passed when building the docs for the toplevel bevy crate.

#[doc(fake_variadic)] only works on tuples and fn pointers, so impls for structs like AnyOf<(T1, T2, ..., Tn)> are unchanged.

Testing

I built the docs locally using RUSTDOCFLAGS='--cfg docsrs' RUSTFLAGS='--cfg docsrs_dep' cargo +nightly doc --no-deps --workspace and checked the documentation page of a trait both in its original crate and the re-exported version in bevy.
The description should correctly mention for how many tuple items the trait is implemented.

I added rustc-args for docs.rs to the bevy crate, I hope there aren't any other notable crates that re-export #[doc(fake_variadic)] traits.


Showcase

bevy_ecs::query::QueryData:
Screenshot 2024-08-12 at 16 41 28

bevy::ecs::query::QueryData (re-export):
Screenshot 2024-08-12 at 16 42 57

Original Description

Resolves #14697

Submitting as a draft for now, very WIP.

Unfortunately, the docs don't show the variadics nicely when looking at reexported items.
For example:

bevy_ecs::bundle::Bundle correctly shows the variadic impl:
image

while bevy::ecs::bundle::Bundle (the reexport) shows all the impls (not good):
image

Built using RUSTDOCFLAGS='--cfg docsrs' cargo +nightly doc --workspace --no-deps (--no-deps because of wgpu-core).

Maybe I missed something or this is a limitation in the totally not private #[doc(fake_variadic)] thingy. In any case I desperately need some sleep now :))

Copy link
Contributor

Welcome, new contributor!

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

0,
12,
P
);

#[cfg(feature = "functions")]
const _: () = {
Copy link
Member

Choose a reason for hiding this comment

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

Any reason not to include the all_tuples! usages in this block? I think it should be doable if we modify the internal macros (e.g. impl_get_ownership!, impl_from_arg!, and impl_into_return!).

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 assume because the block is gated by #[cfg(feature = "functions")]? I'm afraid that I'm the wrong person to answer this question, I saw this code for the first time :)

@alice-i-cecile alice-i-cecile added A-ECS Entities, components, systems, and events C-Docs An addition or correction to our documentation A-App Bevy apps and plugins X-Contentious There are nontrivial implications that should be thought through S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged labels Aug 11, 2024
@alice-i-cecile
Copy link
Member

That's a shame that re-exports don't work. IMO this isn't worth merging until that's fixed here or upstream.

@bash
Copy link
Contributor Author

bash commented Aug 11, 2024

I may have found the culprit: rust-lang/cargo#8811
An "easy" workaround would be to add a rustc arg for docs.rs:

[package.metadata.docs.rs]
rustc-args = ["--cfg", "bevy_docsrs"] # not `docsrs` because web-time fails to compile with --cfg docsrs 🤷🏼 

Also TIL that https://package.metadata.docs.rs redirects to the correct documentation page, how cool is that? ✨

@bash bash marked this pull request as ready for review August 12, 2024 16:09
crates/bevy_utils/macros/src/lib.rs Outdated Show resolved Hide resolved
@janhohenheim janhohenheim added S-Needs-Review Needs reviewer attention (from anyone!) to move forward and removed S-Waiting-on-Author The author needs to make changes or address concerns before this can be merged labels Aug 12, 2024
@alice-i-cecile alice-i-cecile added the M-Needs-Release-Note Work that should be called out in the blog due to impact label Aug 12, 2024
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.

Great use of comments and docs here: explaining why we're doing such a weird thing is really important. Now that re-exports are working I'm happy to merge this! We'll verify this on dev-docs.bevyengine.org before shipping :)

@alice-i-cecile alice-i-cecile added this pull request to the merge queue Aug 12, 2024
@alice-i-cecile alice-i-cecile 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 Needs reviewer attention (from anyone!) to move forward labels Aug 12, 2024
Merged via the queue into bevyengine:main with commit aab1f8e Aug 12, 2024
33 checks passed
@bash bash deleted the fake-variadics branch August 12, 2024 19:20
github-merge-queue bot pushed a commit that referenced this pull request Aug 14, 2024
# Objective

This PR is a follow-up to #14703. I forgot to also add the flags from
`package.metadata.docs.rs` to the docs build.

## Solution

As [discussed on
Discord](https://discord.com/channels/691052431525675048/1272629407659589663/1272643734260940940),
I added the missing flags to `docs.yml`.

I also updated `rustc-args` to properly make use of the array (I think
the value has to be either a space-separated string *or* an array of
strings but not an array of space-separated strings 😆)
rparrett pushed a commit to rparrett/bevy that referenced this pull request Sep 2, 2024
…vyengine#14962)

# Objective

Make the documentation for `SystemParamBuilder` nicer by combining the
tuple implementations into a single line of documentation.

## Solution

Use `#[doc(fake_variadic)]` for `SystemParamBuilder` tuple impls.


![image](https://github.com/user-attachments/assets/b4665861-c405-467f-b30b-82b4b1d99bf7)

(This got missed originally because bevyengine#14050 and bevyengine#14703 were open at the
same time.)
@alice-i-cecile
Copy link
Member

Thank you to everyone involved with the authoring or reviewing of this PR! This work is relatively important and needs release notes! Head over to bevyengine/bevy-website#1672 if you'd like to help out.

@alice-i-cecile alice-i-cecile removed the M-Needs-Release-Note Work that should be called out in the blog due to impact label Oct 20, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-App Bevy apps and plugins 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 X-Contentious There are nontrivial implications that should be thought through
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Consider using fake_variadic to improve docs
4 participants