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

Omit version constraints if the surrounding scope already guards these #1080

Merged
merged 4 commits into from
Mar 26, 2021

Conversation

MarijnS95
Copy link
Contributor

Originally committed and discussed in #991 (comment) / #991 (comment), these patches can finally be submitted as rustdoc automatically propagates feature requirements on modules to the items within it, showing it conveniently on the top of the page without a single doc(cfg()) residing inside this file anymore:

image

Note that rustdoc automatically coalesces to this already! This PR just cleans up the code so that it's easier to read through: a loose #[cfg] statement makes it seem like something special is going on, even though it is simply repeating what is already declared on the mod as a whole. See: https://github.com/MarijnS95/gtk-rs/compare/deduplicate-cfg-guards

Unfortunately a bug with publicly reexporting private modules (ie. our mod auto; pub use crate::auto::*;) prevents rustdoc from figuring out this type constraint on structs, and instead opts to propagate the requirement to each and every member :( (#1079):

image

That is already the case on master (see it for yourself!) and this PR does nothing to alleviate the situation.

@sdroege sdroege requested a review from GuillaumeGomez March 24, 2021 08:37
Copy link
Member

@GuillaumeGomez GuillaumeGomez left a comment

Choose a reason for hiding this comment

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

Nice!

@MarijnS95
Copy link
Contributor Author

@GuillaumeGomez Thanks! Upon re-reading the PR description, do note that those screenshots do not show the current situation! This PR only cleans up the code but the main issue is either solved by applying the temporary workaround mentioned in #1079 or automagically when it is addressed in rustdoc: rust-lang/rust#83428. Apologies for selling "misleading" pretty pictures 😅

@MarijnS95 MarijnS95 force-pushed the deduplicate-cfg-guards branch from 3840eae to 6b08412 Compare March 25, 2021 14:52
This removes unnecessary (duplicate) `#[cfg]` attributes on `fn`s where
the surrounding `impl` block (in `enums`/`flags`) or `mod` (when in a
file that fully defines a `struct`) already declares this constraint.
When the feature requirement is already described it is superfluous (and
noisy) to specify it again.  Besides, rustdoc omits duplicate feature
requirements if it can show this just once at the top of the page for
the entire `struct`.
This removes unnecessary (duplicate) `#[cfg]` attributes on trait
implementations where the surrounding `mod` (when in a file that fully
defines a `struct`) already declares this constraint.  This does not
suffice for `flags`/`enums` where multiple types live in a single shared
file without surrounding constraint.  When the feature requirement is
already described it is superfluous (and noisy) to specify it again.
Besides, rustdoc omits duplicate feature requirements if it can show
this just once at the top of the page for the entire `struct`.
This removes unnecessary (duplicate) `#[cfg]` attributes on `use`
imports where the surrounding `mod` (when in a file that fully defines a
`struct`) already declares this constraint.  This does not suffice for
`flags`/`enums` where multiple types live in a single shared file
without surrounding constraint.  When the feature requirement is already
described it is superfluous (and noisy) to specify it again.
@MarijnS95 MarijnS95 force-pushed the deduplicate-cfg-guards branch from 6b08412 to 71e84df Compare March 26, 2021 13:21
@sdroege sdroege merged commit 56b95ac into gtk-rs:master Mar 26, 2021
@MarijnS95 MarijnS95 deleted the deduplicate-cfg-guards branch March 26, 2021 14:19
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.

3 participants