-
Notifications
You must be signed in to change notification settings - Fork 1.6k
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
RFC: Cargo feature descriptions #3485
base: master
Are you sure you want to change the base?
Conversation
@rustbot label +t-cargo |
I directly applied my more minor feedback that did not change the characteristic of the RFC.
@tgross35 thanks for the access. Definitely makes it handy for the more trivial feedback. |
- Rather than being consistent with `rustdoc` and accepting markdown, should the | ||
`doc` key be consistent with `package.description` and only support plain |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Wanted to highlight this for discussion. My main interest is in being able to show summaries in cargo add
. Might be good to reach out to @kornelski for what they have seen of how features are documented through the ecosystem as that might help show potential requirements.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Package descriptions tend to use markdown `code`
and *emphasis*. Rust devs really like using `
everywhere. Even rustc uses `
in terminal error messages.
Markdown's goal is to look fine even when displayed as plain text.
You could define it as the first line being for CLI help, and the rest for docs. Analogous to how rustdoc handles doc comments.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@kornelski 👍 for using markdown. And I think it makes sense to treat the first logical line (anything before the first double-newline) as a "short description", cutting off subsequent paragraphs in places where full documentation doesn't fit.
text/3485-feature-documentation.md
Outdated
|
||
This RFC describes a new key to under `features` in `Cargo.toml` for | ||
documentation. This will allow Cargo to display this information to the user and | ||
provide a way for `rustdoc` to eventually render this data (how this is rendered |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One of the issues is how rustdoc consumes the data. rust doc generally knows nothing about Cargo.toml
. I would suggest taking #3123 as a reference to start a discussion on cargo-rustdoc integration of this. It doesn't need to be perfect but at least two teams should have some consensus.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did bring it up when I initially proposed this feature, https://rust-lang.zulipchat.com/#narrow/stream/266220-rustdoc/topic/Descriptions.20for.20feature.20flags and then opened a draft RFC suggesting that rustdoc
accept JSON configuration, which Cargo could pass it #3421. That didn't get too much traction, though. I will start that discussion back up
This is allowed in [`rust-lang/rust`](https://github.com/rust-lang/rust/blob/62d9034a0d571b78e518727d6cb4b090569e5238/triagebot.toml#L12). Others have also expected it to work here: - rust-lang#3490 (comment) - rust-lang#3485 (comment) @rustbot label not-rfc
Since RFC 3416 is in FCP, that unblocks this feature. I'm going to go ahead and propose FCP here, and see how close we are to consensus. Personally, I do think we should use markdown here. Markdown already looks reasonable in plain text, by design, and it gives crates.io and docs.rs and rustdoc something nicer to work with. |
@rfcbot merge |
Team member @joshtriplett has proposed to merge this. The next step is review by the rest of the tagged team members: Concerns:
Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up! See this document for info about what commands tagged team members can give me. |
@rfcbot reviewed Though I still want to call out that intra-doc links are important #3416 (review). CC @GuillaumeGomez if they have any opinion on this. |
This is a very good point. Thanks for the ping! |
# Tables are preferred for longer descriptions | ||
[features.corge] | ||
enables = ["bar", "baz"] | ||
doc = """ |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Another good question raised was "should we also support intra-doc links in this documentation?". I personally think we should and make the context the same as the crate top-level. What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ah I see you mentioned it below, my bad.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for the response!
The follow-up question from me would be: Is there any compatibility issues if we hadn't implemented this RFC and rustdoc change all together? If not then this RFC can safely go first.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure. From cargo perspective, whether there are intra-doc links or not in this documentation doesn't matter. But it'll definitely need to be mentioned when support will be discussed in rustdoc.
For the current case, I think cargo
should mention that intra-doc links may be supported when rustdoc support this option and that's it. What do you think?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good to me.
@tgross35 could you add something like this? (or whichever way you'd like to rephase this)
- Rustdoc can build on this to show feature documentation.
+ Rustdoc can build on this to show feature documentation.
+ If this RFC gets stabilized before any corresponding change in rustdoc,
+ its documentation should highlight that rustdoc may parse the description and support intra-doc links in the feature.
+ Users need to be aware of this potential incompatibility.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added, thank you!
|
||
[guide-level-explanation]: #guide-level-explanation | ||
|
||
A new `doc` key will be allowed within a feature's table. This key provides a |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Naming (not finding the past discussion on it)
doc
mirrors#[doc(...)]
description
mirrorspackage.description
documentation
mirrorspackage.documentation
(but that is a URL)
We also tend to not use abbreviations as much. For example, for public-private dependencies, we discussed using pub
vs public
and went with public
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
@epage In my opinion, I think this should be doc
.
I think we should encourage using this not just for a "description" of a feature but for "documentation" for a feature, and that's supported by the idea of it showing up in rustdoc and allowing markdown.
And I think there's a big difference in length between doc
and documentation
, compared to the difference between pub
and public
. (I personally would have gone with pub
for that too, based on widespread usage of pub
within Rust, but I also think public
was less obtrusive because it's fewer characters.)
It's hard enough to get people to write documentation; let's not have any extra friction.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(I personally would have gone with pub for that too, based on widespread usage of pub within Rust, but I also think public was less obtrusive because it's fewer characters.)
Personally, I'd prefer we be consistent and not just focusing on character count.
It's hard enough to get people to write documentation; let's not have any extra friction.
We've shot well past that by requiring a "long form" just to write documentation, hurting both discoverability, character count, and ease of not typing some of those characters on international keyboards.
@rfcbot concern naming See my comment |
Co-authored-by: Josh Triplett <[email protected]>
Co-authored-by: Ed Page <[email protected]>
Co-authored-by: Ed Page <[email protected]>
Co-authored-by: Josh Triplett <[email protected]>
Co-authored-by: Ed Page <[email protected]>
This technically avoids promising work from the @rfcbot concern docs-included-in-design (Anyone should feel free to resolve this concern if members of those teams have been consulted.) |
In the case of The big discussion will actually be: how do we display this information in the generated documentation? |
I do follow this PR from the sidelines, though not all the discussion on here. ( also, @GuillaumeGomez already said what I wanted to say) |
@rfcbot resolve docs-included-in-design |
Tangentially: I hope this information might also become available in rustdoc JSON one day, for the benefit of tools like |
You can ping me directly if it's not there but it's present in the HTML doc. I'll add it as fast as I can if I forgot. ;) |
and following from there, what happens to the docs.rs view, especially since we also have rust-lang/docs.rs#2530 and rust-lang/docs.rs#2531 |
Does I had a draft RFC for a schema to give
I preemptively opened a tracking issue for
I don't know what would happen to the |
Easy answer: same as rustc (so no as far as I know).
👍
docs.rs and rustdoc are tangent. They can both provide an information in different ways. |
So while this isn't relevant for #3421, this is a good point more generally and one we overlooked in #3416. We need to decide if we want to include this in @rfcbot concern cargo-metadata |
The most elegant schema would be to mirror these tables directly into metadata. That is, update: "features": {
"foo": ["bar"],
"bar": [],
"baz": []
} To be something like: "features": {
"foo": { "enables": ["bar"], "doc": "Enable wxyz" },
// ^ bikeshed still being built
"bar": { "enables": [], "doc": "Make use of ..." },
"baz": { "enables": [] } // < only having one serialization seems easier than allowing
// `"bar": []`, but doesn't matter much
} However; being that Cargo needs to support all possible schema versions forever (as I understand it), it seems like the barrier for a breaking schema change is quite high. So maybe we should add a new |
why is this PR still open if the parent is closed? |
The RFC you linked only establishes a format for adding more metadata to features, it doesn't actually confirm that we want to add anything specific. This RFC is about adding documentation as part of metadata, but needs resolution to the two listed concerns. @epage does #3485 (comment) sound reasonable? If so, I will update the RFC. If accepted here we may want to consider updating #3416 so that the feature schema isn't provided only in the documentation RFC - obviously up to you to weigh that vs. updating an accepted RFC. |
As these details seem to be implicit, you are saying this would be a new Yes, 'wed need to support generating data for all possible schemas for ever. Pulled in your ideas and added some more. Mind adding this to the RFC alternatives and documenting which you think we should go with and why? We can then discuss it in a cargo team meeting. A new message format versionWe either deprecate the current version, making new features all-or-nothing, or need to add features to both versions As the format-version is defaulted (with a warning), we'd need to consider what the migration path would be to v2. A parallel table for just thisThis is your We need to do this for every new field. While #3663 was closed, doing something like that would put coupled data in parallel tables. A new
|
Rendered
RFC for
feature-documentation
RFC goals: add a way to write feature descriptions in Cargo.toml
This was split from #3416
This would resolve rust-lang/cargo#4956
FCP