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

Enabled docs.rs feature gate flag rendering. #304

Merged
merged 2 commits into from
Sep 6, 2022

Conversation

metasim
Copy link
Contributor

@metasim metasim commented Sep 2, 2022

  • I agree to follow the project's code of conduct.
  • I added an entry to CHANGES.md if knowledge of this change could be valuable to users.

Followed approach outlined here.

Rendering can be tested locally with:

RUSTDOCFLAGS="--cfg docsrs" cargo +nightly doc --features array --open

Not sure how to test end-to-end (as generated by docs.rs) from a PR.

Closes #228

Sample:

image

@michaelkirk
Copy link
Member

michaelkirk commented Sep 2, 2022

I started using #![cfg_attr(docsrs, feature(doc_auto_cfg))] in WKT which seems to do the trick, and be a little less tedious/error prone.

e.g.

Code: https://github.com/georust/wkt/blob/main/src/lib.rs#L118
Docs: https://docs.rs/wkt/latest/wkt/deserialize/index.html
Doc Screenshot:
Screen Shot 2022-09-02 at 4 51 01 PM

# include `array` feature in documentation
features = ["array"]
# define attribute `docsrs` for feature badges
rustdoc-args = ["--cfg", "docsrs"]
Copy link
Member

Choose a reason for hiding this comment

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

We actually had this before, but it got dropped in #281 (which should probably be reverted at some point).

@rmanoka
Copy link
Contributor

rmanoka commented Sep 3, 2022

@lnicola Note that even though docs.rs now supports the rustc-args configuration, this still means that every dependent crate must include this configuration in their Cargo.toml or that crate's docs won't build. With the dynamic code-gen, we are a bit more accomodating, so we should not revert that PR IMO.

@lnicola
Copy link
Member

lnicola commented Sep 3, 2022

You're right, it's too bad docs.rs doesn't have a solution for this.

@rmanoka
Copy link
Contributor

rmanoka commented Sep 3, 2022

Guess it would be great if they could always set cfg(docsrs) as a sort of a standard signal.

@metasim
Copy link
Contributor Author

metasim commented Sep 3, 2022

This is the first time I've worked with this feature, so some of the nuances are going over my head. Based on what's said above, do we need to pause this PR for a while until the "every dependent crate must include this configuration" issue is resolved (so as to be good citizens)?

As a secondary question, I read about the approach @michaelkirk uses in WKT, but wasn't sure if that would have other consequences, but it certainly seems better. Since WKT is very much a foundational library, should a) we go that way, and b) does it also suffer from the "transitive config" issue?

@lnicola
Copy link
Member

lnicola commented Sep 3, 2022

I don't think this affects the other crates? It only enables the ndarray feature, which I'd expect to work pretty much anywhere.

The previous problem was bigger, since you actually needed GDAL to build the docs.

@rmanoka
Copy link
Contributor

rmanoka commented Sep 3, 2022

Agreed; I was trying to explain why #281 should not be reverted. This PR does not affect the dependent crates' docs, so we're good.

@metasim metasim force-pushed the feature/doc-features branch from 90dea75 to 9001832 Compare September 6, 2022 12:53
@metasim metasim merged commit 7d3dfed into georust:master Sep 6, 2022
@metasim metasim deleted the feature/doc-features branch September 6, 2022 18:26
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.rs should include array feature
4 participants