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

Recommend having document-features as an optional dependency #8

Closed
wants to merge 4 commits into from

Conversation

emilk
Copy link

@emilk emilk commented Jun 10, 2022

Closes #7

There are two ways to successfully use document-features. One is to to have it as a mandatory dependency:

document-features = "0.2" + #![doc = document_features::document_features!()]

However, this adds a dependency that is only needed when building docs, which is unnecessary. The other way is to have it optional:

[package.metadata.docs.rs]
all-features = true

[dependencies]
document-features = { version = "0.2", optional = true }
#![cfg_attr(feature = "document-features", doc = document_features::document_features!())]

This is more verbose, but means the document-features doesn't become a build dependency, which is great.

Unfortunately, the old docs had a mix with document-features = { version = "0.2", optional = true } + #![doc = document_features::document_features!()] which fails when you do cargo check.


In this PR I suggest we always use the more verbose (but in my opinion better) way: the optional dependency.

@tronical
Copy link
Member

Just a thought, I might be missing something: If we recommend having document-features optional by default, doesn't that mean that the developer workflow of running cargo doc will not yield the desired result anymore?

If that's the case, then the docs should perhaps explain that cargo doc needs to be invoked with that feature?

Also, with that change, isn't the compatibility section obsolete now?

@ogoffart
Copy link
Member

Sorry for the delayed answer.
Thanks for this PR.

I agree with @tronical that the problem is that if we suggest to make it optional by default, then cargo doc no longer works by default.
IMHO the choice to have it optional should basically be done by the crate user anyway. Do we want to have this crate always optional? Currently, the vast majority of the crates have it optional: https://lib.rs/crates/document-features/rev

But if the problem is just that the docs seems to induce people in error as they copy-paste the dependency line from the doc, maybe we can just fix that one point instead.

@emilk
Copy link
Author

emilk commented Jul 25, 2022

That's the rub: should the docs nudge users towards having document-features as an optional dependency, or as a default dependency?

I prefer the first (hence this PR). I agree though that we should also document that one would need cargo doc --all-features to then get the document-features documentation. But honestly, I think using --all-features with cargo doc is always a good idea anyway :)

@emilk
Copy link
Author

emilk commented Sep 15, 2022

So since a83f3c4 the docs now always suggests having document-features as a non-optional feature. This is good, since it should cause a lot less confusion.

This PR suggest flipping this to always suggests having document-features as an optional feature. This has some advantages (no transient dependency on document-features for other crates) but also some downsides (more verbose, cargo doc requires --all-features to work).

What say you, maintainers?

Copy link
Member

@ogoffart ogoffart left a comment

Choose a reason for hiding this comment

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

I don't think document-feature should necessarily be optional. It's a fairly lightweight dependency.

Also, the user will have to remember to enable the feature when then run cargo doc themselves. And what about users of user's library, they should also be told to enable the feature when running the docs.

README.md Outdated Show resolved Hide resolved
lib.rs Outdated Show resolved Hide resolved
lib.rs Outdated Show resolved Hide resolved
README.md Outdated Show resolved Hide resolved
@emilk emilk changed the title Make default example compatible with cargo check Recommend having document-features as an optional dependency Sep 15, 2022
@emilk
Copy link
Author

emilk commented Jan 11, 2024

Belated agreement: having document-features as an optional dependency is a bit annoying, and only for people who really hate added dependencies.

@emilk emilk closed this Jan 11, 2024
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.

Docs suggests document-features as an optional feature
3 participants