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

Apply #[doc(cfg(feature = "..."))] banners in docs #734

Closed
wants to merge 6 commits into from

Conversation

jfrimmel
Copy link

@jfrimmel jfrimmel commented Dec 15, 2019

This PR adds the #[doc(cfg(..))] banners for all #[cfg(..)] attributes. The result can be seen here:
image

This is achieved by providing a new config option called syn_enable_doc_cfg, that is either set by the build script, if a nightly compiler is used, or from docs.rs directly (via crate metadata).

Closes #731.

To be able to include banners, which feature is needed for an item, the
feature `#![feature(doc_cfg)]` has to be available. This feture is only
available in nightly compiler since August 2017. So we need to detect,
whether we are building the docs under a nightly compiler to add the
doc-cfg-banners.
This enables the usage of the #[doc(cfg(...))] attributes.
build.rs Outdated Show resolved Hide resolved
src/expr.rs Outdated Show resolved Hide resolved
src/lib.rs Outdated Show resolved Hide resolved
Copy link
Collaborator

@mystor mystor left a comment

Choose a reason for hiding this comment

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

It seems like you're attaching attributes to everything with a cfg on it. We don't need to do that, as most items which you're attaching the attribute to are not exported at all, and so are never documented.

In general, we probably don't want to attach this attribute to anything which doesn't already have a doc-comment on it.

src/attr.rs Outdated Show resolved Hide resolved
src/attr.rs Outdated Show resolved Hide resolved
src/attr.rs Outdated Show resolved Hide resolved
src/custom_keyword.rs Outdated Show resolved Hide resolved
src/data.rs Outdated Show resolved Hide resolved
src/gen/fold.rs Outdated Show resolved Hide resolved
@jfrimmel
Copy link
Author

I will address the issues and force-push once I'm done.

Copy link
Author

@jfrimmel jfrimmel left a comment

Choose a reason for hiding this comment

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

Now the #[doc(cfg(..))]-attributes are only attached to public items.

It is ready for review again.

@jfrimmel jfrimmel requested a review from mystor December 16, 2019 19:06
Copy link
Collaborator

@mystor mystor 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 added some more comments about how we can simplify these. I also think there's a good chance we can avoid manually writing it out all over the place by getting the ast_struct macros to add them for us.

We should also probably put these attributes on the generated Fold, Visit and VisitMut traits. We can do that in the codegen crate, and then re-run it: (visit, fold, visit_mut)

src/attr.rs Outdated Show resolved Hide resolved
@@ -232,6 +236,7 @@ ast_struct! {
/// A slice literal expression: `[a, b, c, d]`.
///
/// *This type is available if Syn is built with the `"full"` feature.*
#[cfg_attr(syn_enable_doc_cfg, doc(cfg(feature = "full")))]
Copy link
Collaborator

@mystor mystor Dec 19, 2019

Choose a reason for hiding this comment

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

These types declared using #full can have the attribute applied by the ast_struct! macro.

syn/src/macros.rs

Lines 6 to 9 in 6968295

#[cfg(feature = "full")]
#[cfg_attr(feature = "extra-traits", derive(Debug, Eq, PartialEq, Hash))]
#[cfg_attr(feature = "clone-impls", derive(Clone))]
$($attrs_pub)* struct $name $($rest)*

I think that every ast_struct!, ast_enum_of_structs! or ast_enum! requires either feature = "derive", any(feature = "derive", feature = "full"), or feature = "full" so we might be able to get away with specifying these attributes (and perhaps also the old available comments?) in that macro. @dtolnay, would you find that cleaner?

Copy link
Owner

Choose a reason for hiding this comment

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

I would like to keep it simple and stick with the current approach in the PR for now.

Copy link
Collaborator

Choose a reason for hiding this comment

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

OK - nevermind on this comment in that case :-)

Copy link
Author

Choose a reason for hiding this comment

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

I thought about sticking the attribute to the expansion of the #full token, but didn't do it in the end. The cause was, that there were some inconsistencies and there is no such token for derive||full.

src/path.rs Outdated Show resolved Hide resolved
src/stmt.rs Outdated Show resolved Hide resolved
build.rs Outdated Show resolved Hide resolved
@dtolnay
Copy link
Owner

dtolnay commented Dec 25, 2019

I gave this a try locally with RUSTDOCFLAGS='--cfg syn_enable_doc_cfg' cargo doc --all-features and... it's not great.

I am happy with how the message turns out at the top of the doc page of a single type or function, as seen in the screenshot at the top of this PR.

But our index page becomes extremely noisy; I wish there were a way to not show all of these:

And inheriting the same note onto every public field seems silly in our use case:

I plan to leave some feedback on the doc_cfg tracking issue and defer this PR until the feature is in better shape. Thanks anyway @jfrimmel!

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.

Apply #[doc(cfg(feature = "..."))] on docs.rs
3 participants