search

slint-ui / document-features

Extract documentation for the feature flags from comments in Cargo.toml
Apache License 2.0
173 stars 13 forks source link

Recommend having `document-features` as an optional dependency #8

Closed emilk closed 10 months ago

emilk commented 2 years ago

Closes https://github.com/slint-ui/document-features/issues/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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

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 commented 2 years ago

So since https://github.com/slint-ui/document-features/commit/a83f3c4bd5bd9d9c0c61a642fe4d7134d197e05b 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?

emilk commented 10 months ago

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