Improve include macro documentation #106453

coastalwhite · 2023-01-04T15:14:08Z

As outlined in #106118, the include! macro is a SEO problem when it comes to the Rust documentation. Beginners may see it as a replacement to include syntax in other languages. I feel like this documentation should quite explicitly link to the modules' documentation.

The primary goal of this PR is to address that issue by adding a warning to the documentation. While I was here, I also added some other parts. This included a Uses section and some (intra doc) links to other relevant topics.

I hope this can help beginners to Rust more quickly understand some multi-file project intricacies.

References

Syntax for the warning: https://github.com/tokio-rs/tracing/blob/58accc6da3f04af3f6144fbe6d68af7225c70c02/tracing/src/lib.rs#L55

rustbot · 2023-01-04T15:14:16Z

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @thomcc (or someone else) soon.

Please see the contribution instructions for more information.

rustbot · 2023-01-04T15:14:18Z

Hey! It looks like you've submitted a new PR for the library teams!

If this PR contains changes to any rust-lang/rust public library APIs then please comment with @rustbot label +T-libs-api -T-libs to tag it appropriately. If this PR contains changes to any unstable APIs please edit the PR description to add a link to the relevant API Change Proposal or create one if you haven't already. If you're unsure where your change falls no worries, just leave it as is and the reviewer will take a look and make a decision to forward on if necessary.

Examples of T-libs-api changes:

Stabilizing library features
Introducing insta-stable changes such as new implementations of existing stable traits on existing stable types
Introducing new or changing existing unstable library APIs (excluding permanently unstable features / features without a tracking issue)
Changing public documentation in ways that create new stability guarantees
Changing observable runtime behavior of library APIs

Noratrieb · 2023-01-04T15:29:07Z

library/core/src/macros/mod.rs

+    /// If the included file is parsed as an expression, it is placed in the surrounding code
+    /// [unhygienically](https://doc.rust-lang.org/reference/macros-by-example.html#hygiene). This
+    /// could result in variables or functions being different from what the file expected if there
+    /// are variables or functions that have the same name in the current file.


This wording makes it sound like it's hygienic when in item context

The latest commit alters the wording on the hygiene of the macro. This should make it clear that even for an item, it is unhygienic.

thomcc · 2023-01-04T16:30:07Z

Can you show a screenshot of what this warning ends up looking like?

thomcc · 2023-01-04T16:30:35Z

@rustbot author

(use at-rustbot ready to push it back into my queue when ready)

coastalwhite · 2023-01-05T16:00:37Z

Can you show a screenshot of what this warning ends up looking like?

Currently, it looks like this.

It is a pretty tame comment box. I tried it with a block quote, but I feel it does not really add the proper weight to it. There are some alternatives. I am not sure which fits best. Maybe the warning with the tooltip is the most appropriate one, since it states "Warning"?

Alternatives

Option 2. Error without tooltip

Option 3. Error with tooltip

Option 4. Warning with tooltip

coastalwhite · 2023-01-05T16:12:22Z

@rustbot ready

Noratrieb · 2023-01-05T16:16:05Z

Are there other places in the std docs where we have similar warnings?

coastalwhite · 2023-01-05T16:40:47Z

Are there other places in the std docs where we have similar warnings?

There are some examples, but none introduce a new semi-<pre> block. I do feel like it is easy to read over it in this case, which warrants the block.

Other examples:

Warning in HashMap::with_hasher (Same for HashSet): No bold, italic, capitalization or header. Just Warning:
NOTE in LinkedList. Capitalization.
Warning in alloc::boxed. All bold.
Warning in std::path::Path. No bold, italic, capitalization or header. Just Warning:
Warning in Ipv4Addr::is_reserved. Header.
Warning in std::process::Child. Header.

There are plenty of other examples, but none introduce a non-pre block. std::boxed does the closest to introducing a block that I could find. Most use a Warning:, capitalization, header or a combination of those things.

Overall, it seems like there isn't a convention on how to do Warning. The same for Note. For some cases, I would argue the block is way better and will prevent people from reading over critical information. It also isn't unheard of to add inline HTML to the documentation (std::pin).

thomcc · 2023-01-06T01:55:22Z

Hm. TBH, this doesn't really feel like a t-libs decision, both because it's a documentation change (and documentation style change), and because include! is really a language feature and not part of the library. I'm going to punt it to the doc team here.

r? docs

GuillaumeGomez · 2023-01-06T10:15:34Z

library/core/src/macros/mod.rs

-    /// way at compile time. So, for instance, an invocation with a Windows path
-    /// containing backslashes `\` would not compile correctly on Unix.
+    /// <div class="example-wrap" style="display:inline-block">
+    /// <pre class="compile_fail" style="white-space:normal;font:inherit;">


I don't think this is the right way to do this because if someone enables the "see code blocks lines", it'll make a very weird output. We are talking about adding such a feature in #79710. I re-started the debate and hopefully, we might be able to have something not too far in the future.

Would you be open to me removing the <div> so that this can be merged? This PR generally improves the documentation, so I see no reason in the html blocking the PR. I will follow the discussion on the better support for warning blocks and update it when the time comes.

Just remove the HTML tags and then it's good for me. We can come back to this at a later time.

GuillaumeGomez · 2023-01-06T13:34:04Z

Thanks!

@bors r+ rollup

bors · 2023-01-06T13:34:06Z

📌 Commit ae667be has been approved by GuillaumeGomez

It is now in the queue for this repository.

…iaskrgr Rollup of 7 pull requests Successful merges: - rust-lang#106287 (Add some docs to `bug`, `span_bug` and `delay_span_bug`) - rust-lang#106341 (refactor: clean up `errors.rs` and `error_codes_check.rs`) - rust-lang#106453 (Improve include macro documentation) - rust-lang#106466 (Fix rustdoc source code rendering for `#[path = "../path/to/mod.rs"]` links) - rust-lang#106528 (Tiny formatting fix) - rust-lang#106534 (rustdoc-gui: Use new block syntax for define-function in goml scripts) - rust-lang#106542 (Add default and latest stable edition to --edition in rustc (attempt 2)) Failed merges: r? `@ghost` `@rustbot` modify labels: rollup

Improve include macro documentation

0c43b42

rustbot assigned thomcc Jan 4, 2023

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Jan 4, 2023

Tidy up whitespace

eb2980c

Noratrieb reviewed Jan 4, 2023

View reviewed changes

rustbot added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jan 4, 2023

Better phrasing for hygiene of include macro

c30f7c9

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jan 5, 2023

rustbot assigned GuillaumeGomez and unassigned thomcc Jan 6, 2023

thomcc added the A-docs Area: Documentation for any part of the project, including the compiler, standard library, and tools label Jan 6, 2023

GuillaumeGomez mentioned this pull request Jan 6, 2023

Add syntax extension to add warning blocks #79710

Closed

GuillaumeGomez reviewed Jan 6, 2023

View reviewed changes

Remove HTML tags around warning

ae667be

GuillaumeGomez approved these changes Jan 6, 2023

View reviewed changes

bors removed the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jan 6, 2023

bors added the S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. label Jan 6, 2023

coastalwhite mentioned this pull request Jan 6, 2023

Include macro should link to modules #106118

Closed

matthiaskrgr mentioned this pull request Jan 6, 2023

Rollup of 7 pull requests #106544

Merged

bors merged commit 72d650f into rust-lang:master Jan 7, 2023

rustbot added this to the 1.68.0 milestone Jan 7, 2023

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Improve include macro documentation #106453

Improve include macro documentation #106453

coastalwhite commented Jan 4, 2023 •

edited

Loading

rustbot commented Jan 4, 2023

rustbot commented Jan 4, 2023

Noratrieb Jan 4, 2023

coastalwhite Jan 5, 2023

thomcc commented Jan 4, 2023

thomcc commented Jan 4, 2023

coastalwhite commented Jan 5, 2023 •

edited

Loading

coastalwhite commented Jan 5, 2023

Noratrieb commented Jan 5, 2023

coastalwhite commented Jan 5, 2023 •

edited

Loading

thomcc commented Jan 6, 2023

GuillaumeGomez Jan 6, 2023

coastalwhite Jan 6, 2023

GuillaumeGomez Jan 6, 2023

coastalwhite Jan 6, 2023

GuillaumeGomez commented Jan 6, 2023

bors commented Jan 6, 2023

Improve include macro documentation #106453

Improve include macro documentation #106453

Conversation

coastalwhite commented Jan 4, 2023 • edited Loading

References

rustbot commented Jan 4, 2023

rustbot commented Jan 4, 2023

Noratrieb Jan 4, 2023

Choose a reason for hiding this comment

coastalwhite Jan 5, 2023

Choose a reason for hiding this comment

thomcc commented Jan 4, 2023

thomcc commented Jan 4, 2023

coastalwhite commented Jan 5, 2023 • edited Loading

Alternatives

Option 2. Error without tooltip

Option 3. Error with tooltip

Option 4. Warning with tooltip

coastalwhite commented Jan 5, 2023

Noratrieb commented Jan 5, 2023

coastalwhite commented Jan 5, 2023 • edited Loading

thomcc commented Jan 6, 2023

GuillaumeGomez Jan 6, 2023

Choose a reason for hiding this comment

coastalwhite Jan 6, 2023

Choose a reason for hiding this comment

GuillaumeGomez Jan 6, 2023

Choose a reason for hiding this comment

coastalwhite Jan 6, 2023

Choose a reason for hiding this comment

GuillaumeGomez commented Jan 6, 2023

bors commented Jan 6, 2023

coastalwhite commented Jan 4, 2023 •

edited

Loading

coastalwhite commented Jan 5, 2023 •

edited

Loading

coastalwhite commented Jan 5, 2023 •

edited

Loading