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

Add warning sections in rustdoc #79677

Closed

Conversation

GuillaumeGomez
Copy link
Member

Fixes #73935.

It looks like this:

Screenshot from 2020-12-03 22-27-21

I used the same style than code blocks in case. :)

r? @jyn514

@rust-highfive
Copy link
Collaborator

Some changes occurred in HTML/CSS/JS.

cc @GuillaumeGomez

A change occurred in the Ayu theme.

cc @Cldfire

Some changes occurred in HTML/CSS themes.

cc @GuillaumeGomez

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Dec 3, 2020
@jyn514 jyn514 added T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. needs-fcp This change is insta-stable, so needs a completed FCP to proceed. labels Dec 3, 2020
Copy link
Member

@jyn514 jyn514 left a comment

Choose a reason for hiding this comment

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

I think it also makes sense to add info sections (like in https://docs.rs/tracing/0.1.22/tracing/span/struct.Span.html#method.record), but that can maybe wait for a follow-up.

Comment on lines 1 to 3
# Documentation rendering tweaks

This chapter is about how to improve the generated documentation appearance.
Copy link
Member

Choose a reason for hiding this comment

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

I would prefer to put this in the 'advanced features' section - I don't expect many people to use it, and having a whole chapter for the one feature seems like overkill, I think people will expect 'how do I write rust documentation' which is not what this is.

Copy link
Member Author

Choose a reason for hiding this comment

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

We have already a lot of things in there, making access to information more complicated. I expect for us to have more "front-end" tweaks like that so I think it's better to have it in a section on its own.

Copy link
Member

Choose a reason for hiding this comment

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

Hmm, if you plan to have more tweaks, does it make sense to plan them out ahead of time? I don't want to land these piece-meal and realize we ended with a bunch of similar ones that don't address people's needs.

@GuillaumeGomez
Copy link
Member Author

I think it also makes sense to add info sections (like in https://docs.rs/tracing/0.1.22/tracing/span/struct.Span.html#method.record), but that can maybe wait for a follow-up.

Good idea, however I'd prefer put it into another PR so that we can have small and focused ones.

@Nemo157
Copy link
Member

Nemo157 commented Dec 4, 2020

section seems wrong for this:

Do not use the <section> element as a generic container; this is what <div> is for, especially when the sectioning is only for styling purposes. A rule of thumb is that a section should logically appear in the outline of a document.
https://developer.mozilla.org/en-US/docs/Web/HTML/Element/section

I also wonder about namespacing the class somehow, to avoid issues with doc pages embedded within other contexts (like docs.rs, which has its own div.warning css).

@GuillaumeGomez
Copy link
Member Author

@Nemo157 It's both very good points. Rustdoc embed its content inside a main element so I'll use this rule too to enforce it to avoid conflicts.

@GuillaumeGomez
Copy link
Member Author

Fixed issues!

@GuillaumeGomez GuillaumeGomez added the A-rustdoc-ui Area: Rustdoc UI (generated HTML) label Dec 4, 2020
@GuillaumeGomez GuillaumeGomez changed the title Warning sections Add warning sections in rustdoc Dec 4, 2020
@GuillaumeGomez
Copy link
Member Author

@rfcbot fcp merge

This feature allows to have an official way in rustdoc to add small text warning blocks. We saw with the @rust-lang/docs-rs team about the class name to prevent potential conflicts with docs.rs style.

@rfcbot
Copy link

rfcbot commented Dec 4, 2020

Team member @GuillaumeGomez 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 rfcbot added proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. labels Dec 4, 2020
Copy link
Member

@jyn514 jyn514 left a comment

Choose a reason for hiding this comment

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

I think it would be great to hear from some authors in the community whether they'd use this or not. Maybe @dtolnay, @Darksonn, @BurntSushi would be interested in giving feedback? The goal is to allow making Notes like https://docs.rs/tracing/0.1.22/tracing/span/struct.Span.html#method.record easy to add and consistent across documentation for different projects.

Comment on lines 1 to 3
# Documentation rendering tweaks

This chapter is about how to improve the generated documentation appearance.
Copy link
Member

Choose a reason for hiding this comment

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

Hmm, if you plan to have more tweaks, does it make sense to plan them out ahead of time? I don't want to land these piece-meal and realize we ended with a bunch of similar ones that don't address people's needs.

@GuillaumeGomez
Copy link
Member Author

Hmm, if you plan to have more tweaks, does it make sense to plan them out ahead of time? I don't want to land these piece-meal and realize we ended with a bunch of similar ones that don't address people's needs.

@jyn514 I expect the class:class to be documented there as well as the "information block". And I think there will be more in the future. Again, I think it's better to not put too much into the "advanced features" chapter, specially in this case considering that a few other "front-end changes" are coming all at once. :)

@Darksonn
Copy link
Contributor

Darksonn commented Dec 4, 2020

I certainly think this seems useful. I would probably use it rather infrequently, but footguns like block_in_place deserve warnings that stand out. We already use a similar feature in the tutorial on our website.

I could also imagine wanting a blue info box for larger pieces of documentation such as the select! macro.

Copy link
Member

@dtolnay dtolnay left a comment

Choose a reason for hiding this comment

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

Exposing this by HTML tags <div class="rustdoc-warning">...</div> in the source file is not appealing to me. I think Phabricator does this better, see https://secure.phabricator.com/book/phabricator/article/remarkup/ where it covers NOTE:, WARNING:, IMPORTANT:.

Basically it would render this as a warning:

/// WARNING: it doesn't work yet.

and remains suitably readable in the source file.

@GuillaumeGomez
Copy link
Member Author

I could also imagine wanting a blue info box for larger pieces of documentation such as the select! macro.

Already scheduled! :D

@dtolnay But then it's not markdown compliant anymore and becomes a rustdoc extension. And I don't really like your suggestion as is. If we go in this direction, why not something like:

/// /!\ this is the warning text /!\

?

@jyn514
Copy link
Member

jyn514 commented Dec 4, 2020

@dtolnay how would that syntax work for multi-line blocks? Are you expecting these to all render as the same warning?

/// WARNING: first line
/// second line, still a warning
///
/// after a line break, no longer part of the `rustdoc-warning` block

@Darksonn
Copy link
Contributor

Darksonn commented Dec 4, 2020

For comparison, this is what the markdown parser we happen to use on the Tokio website accepts:

[[info]]
| Many of Tokio's types are named the same as their synchronous equivalent in
| the Rust standard library. When it makes sense, Tokio exposes the same APIs
| as `std` but using `async fn`.

I don't exactly love this syntax, but I find it somewhat better than the html tag.

@GuillaumeGomez
Copy link
Member Author

So, adding an extension to rust doc comment syntax to generate a warning block seems like another feature. I'm not really convinced with the suggestions, and in the end, we still need to generate the HTML we have here. :)

@jyn514
Copy link
Member

jyn514 commented Dec 4, 2020

@GuillaumeGomez well, the syntax is important to know before stabilizing. If you want to make this available on nightly only I'm ok with that, but making this insta-stable means the HTML syntax can't be removed, and people have already raised concerns that it could be improved.

@jyn514
Copy link
Member

jyn514 commented Dec 4, 2020

@rfcbot concern what syntax should be used?

@GuillaumeGomez
Copy link
Member Author

For me it's another debate. In the end, we still want to generate his HTML which needs to be present before we even add the syntax extension. I think it's two different issues.

@GuillaumeGomez
Copy link
Member Author

@rfcbot rfc cancel

@GuillaumeGomez
Copy link
Member Author

I opened #79710 to have the debate over the syntax extension somewhere else. We discussed about it with @jyn514 for quite some time and decided to not document the current behavior and to only document the syntax extension once approved to not enforce the usage of HTML directly.

@crlf0710 crlf0710 added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. S-blocked Status: Blocked on something else such as an RFC or other implementation work. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. needs-fcp This change is insta-stable, so needs a completed FCP to proceed. proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. labels Jan 15, 2021
@camelid
Copy link
Member

camelid commented Feb 14, 2021

@rfcbot rfc cancel

I don't think this worked. Instead you need @rfcbot cancel.

@jyn514
Copy link
Member

jyn514 commented Feb 14, 2021

FWIW I am against even putting this on nightly until we decide on a syntax, it means we'll be giving precedence to that syntax over the others and there will be pressure to stabilize that syntax because it exists and people are using it. This is especially bad for rustdoc because docs.rs uses nightly by default.

@camelid
Copy link
Member

camelid commented Feb 15, 2021

Canceling FCP since it's already de facto cancelled:

@rfcbot cancel

@rfcbot
Copy link

rfcbot commented Feb 15, 2021

@camelid proposal cancelled.

@camelid camelid added A-rustdoc-ui Area: Rustdoc UI (generated HTML) S-blocked Status: Blocked on something else such as an RFC or other implementation work. and removed A-rustdoc-ui Area: Rustdoc UI (generated HTML) S-blocked Status: Blocked on something else such as an RFC or other implementation work. labels Mar 12, 2021
@bors
Copy link
Contributor

bors commented May 10, 2021

☔ The latest upstream changes (presumably #85156) made this pull request unmergeable. Please resolve the merge conflicts.

@camelid camelid added S-blocked Status: Blocked on something else such as an RFC or other implementation work. and removed S-blocked Status: Blocked on something else such as an RFC or other implementation work. labels May 28, 2021
@camelid
Copy link
Member

camelid commented Dec 10, 2021

I'm going to close this for now in favor of #79710. Once we decide on a path forward, this can be re-opened or a new PR can be created.

@camelid camelid closed this Dec 10, 2021
@GuillaumeGomez GuillaumeGomez deleted the warning-sections branch January 24, 2023 15:34
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-rustdoc-ui Area: Rustdoc UI (generated HTML) S-blocked Status: Blocked on something else such as an RFC or other implementation work. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Provide a way to add warning blocks in documentation.
10 participants