Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
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 doesn't seem specific, should we change this too?
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.
Are you referring to the
[GitHub](https://github.com/rust-lang/rust)
link?If so, yeah, I was working toward suggesting a PR for that too but it was a bit less "obvious" fix so was limiting this PR to just the one change.
The main question/issue I see is that the "Writing Documentation" section includes this sentence:
So, depending on what "this documentation" is intended to refer to it's unclear whether the best approach is:
library/std/src/lib.rs
src/doc
in the tree, and standard API documentation is generated from the source code itself (e.g.lib.rs
)." (And/or make the link additions to https://rustc-dev-guide.rust-lang.org/contributing.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.
I think you could just include another suggestion here along with the patch. But I think linking to
library/std/src/lib.rs
isn't good enough since people may want to edit other files.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've put a PR in the dev guide repo with option 3: rust-lang/rustc-dev-guide#898
If that PR gets merged then perhaps we can decide if there's an issue with duplicating the content here?
The example link to
library/std/src/lib.rs
was (a) the literal source for the documentation in question :) and (b) just intended as an example to help people start looking in the right place. Open to other suggestions of where to link instead.Adding multiple commits to a PR via the GitHub interface (as I'm currently doing for such "drive-by" contributions) is complicated due to the inability to--if I recall correctly--squash commits as required by Rust? (Or maybe that's a per-repo configuration setting?) So, if it's possible to just fix the one issue in this PR, that would be preferable on my part. :)
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.
See also rust-lang/triagebot#821
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 it makes sense to have the changes in one place so it's easier to find all the discussion. I won't block on squashing the commits :)
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.
Approach A
Based on the now merged #898, one possible approach is something like:
But if this file is being viewed via https://doc.rust-lang.org/std/index.html#contributing-changes-to-the-documentation then that wording seems potentially confusing?
Approach B
An alternate wording:
(Given the content of the latter paragraph is now already mentioned elsewhere, I'm not sure that it's necessarily justified to mention it here.)
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.
Hmm, that doesn't seem too confusing to me: It says the documentation for the standard library lives in the source code for the standard library. I think
lib.rs
is a little confusing though, maybe saylibrary/std
instead.Both of your approaches seem fine to me, I think both are more clear than the current links.