-
Notifications
You must be signed in to change notification settings - Fork 12.9k
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
Added doc on keyword Pub. #69723
Added doc on keyword Pub. #69723
Conversation
It looks like you accidentally created the file at the top-level directory, instead of editing the existing file at Let me know if I can help further! |
Hi, |
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
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.
Looking good. Some minor nits, let me know if I can help fix them.
Hi, Mark. |
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
src/libstd/keyword_docs.rs
Outdated
/// For more information on the `pub` keyword, please see the [module] section | ||
/// of the Rust Book detailing paths. Or the [visibility] section of Rust By Example. | ||
/// [module]: ../book/ch7-3-paths-for-referring-to-an-item-in-the-module-tree.html | ||
/// [visibility]:../rust-by-example/mod/visibility.html | ||
/// [not yet complete]: https://github.com/rust-lang/rust/issues/34601 |
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.
This is no longer used, so can be dropped.
src/libstd/keyword_docs.rs
Outdated
/// | ||
/// For more information on the `pub` keyword, please see the [module] section |
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 think linking to https://doc.rust-lang.org/reference/visibility-and-privacy.html is a better fit here, it seems like the most exhaustive and best documentation we have right now.
src/libstd/keyword_docs.rs
Outdated
/// of external modules. The `pub` keyword may also be used in a `use` declaration to re-export | ||
/// an identifier from a namespace. | ||
/// | ||
/// In the below example we define `PublicStruct` within `public_inner_module` and make both 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.
In thinking about this some more, I would personally prefer to drop the code examples from this documentation and primarily lead people towards the visibility documentation in the reference.
I think in practice that means leaving the first paragraph in, and then deleting the code examples and such up to the final paragraph. I think that this is just not the best place for long-form documentation like this.
Thanks, Mark. |
src/libstd/keyword_docs.rs
Outdated
/// For more information on the `pub` keyword, please see the [visibility] section | ||
/// of the Reference. Or the [visibility] section of Rust By Example. | ||
/// [visibility]: ../reference/visibility-and-privacy.html?highlight=pub#visibility-and-privacy | ||
/// [visibility]:../rust-by-example/mod/visibility.html |
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.
These will need to be distinct identifiers; one option is to reword the above to be: please see the visibility sections of the [Reference] and examples in the [Rust by Example] collection.
.
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
The job Click to expand the log.
I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact |
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 think the blank line in between was actually the problem though not sure :)
I'm not sure either! Always exciting to see what Tina has to say. :D |
This is looking great! I think the last thing to do is to squash the commits into one. I would recommend to use If you want I can also do this for you and push up to this branch, or alternatively feel free to ping me on Zulip or Discord ( |
Hi, Mark. Thanks so much for your help! |
No problem! @bors r+ rollup=always |
📌 Commit b27a77ee7310dc7bd5044898388dc00a0464c886 has been approved by |
🌲 The tree is currently closed for pull requests below priority 1000, this pull request will be tested once the tree is reopened |
@bors r+ rollup=always |
📌 Commit 87f8ee6 has been approved by |
🌲 The tree is currently closed for pull requests below priority 1000, this pull request will be tested once the tree is reopened |
…acrum Added doc on keyword Pub. Hi, this is my first pull request. I hope it's OK. Please let me know if it would benefit from any changes. Thank you.
Rollup of 12 pull requests Successful merges: - #69403 (Implement `Copy` for `IoSlice`) - #69460 (Move some `build-pass` tests to `check-pass`) - #69723 (Added doc on keyword Pub.) - #69802 (fix more clippy findings) - #69809 (remove lifetimes that can be elided (clippy::needless_lifetimes)) - #69947 (Clean up E0423 explanation) - #69949 (triagebot.toml: add ping aliases) - #69954 (rename panic_if_ intrinsics to assert_) - #69960 (miri engine: fix treatment of abort intrinsic) - #69966 (Add more regression tests) - #69973 (Update stable-since version for const_int_conversion) - #69974 (Clean up E0434 explanation) Failed merges: r? @ghost
Hi, this is my first pull request. I hope it's OK. Please let me know if it would benefit from any changes. Thank you.