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.

Sign up for GitHub

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

Jump to bottom

Format std::env::consts docstrings with markdown backticks #128535

Merged
merged 3 commits into from
Sep 17, 2024
Merged

Format std::env::consts docstrings with markdown backticks #128535

merged 3 commits into from
Sep 17, 2024

Conversation

mmvanheusden
Copy link
Contributor

This clarifies possible outputs the constants might be.

Before:

After:

@rustbot
Copy link
Collaborator

rustbot commented Aug 2, 2024

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @workingjubilee (or someone else) some time within the next two weeks.

Please see the contribution instructions for more information. Namely, in order to ensure the minimum review times lag, PR authors and assigned reviewers should ensure that the review label (S-waiting-on-review and S-waiting-on-author) stays updated, invoking these commands when appropriate:

  • @rustbot author: the review is finished, PR author should check the comments and take action accordingly
  • @rustbot review: the author is ready for a review, this PR will be queued again in the reviewer's queue

@rustbot 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 Aug 2, 2024
workingjubilee
workingjubilee requested changes Aug 6, 2024
View reviewed changes
Copy link
Member

@workingjubilee workingjubilee left a comment

Choose a reason for hiding this comment

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

This is not correct. None of these are actual values in Rust source code, and the backticks are for code, therefore adding the backticks here would also require adding the string quotation marks for each case.

@rustbot 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 Aug 6, 2024
@workingjubilee
Copy link
Member

If it is purely for readability's sake then I do not find this to be more readable.

@mmvanheusden
Copy link
Contributor Author

Sure, will change that.

Would it be okay if I add each value it can return to the docs? Or would that list become too long

@workingjubilee
Copy link
Member

It would become far too long for some of these. I think we should only have the "top 5" in the docs, honestly.

I would be okay with adding the entire list inside <details></details> blocks, for the curious.

@workingjubilee
Copy link
Member

I would be okay with adding the entire list inside <details></details> blocks, for the curious.

This would look vaguely like

/// <details><summary>Full List</summary>
///
/// - item
/// - item
///
/// </details>

@rustbot rustbot added the has-merge-commits PR has merge commits, merge with caution. label Aug 15, 2024
@rustbot
Copy link
Collaborator

rustbot commented Aug 15, 2024

There are merge commits (commits with multiple parents) in your changes. We have a no merge policy so these commits will need to be removed for this pull request to be merged.

You can start a rebase with the following commands:

$ # rebase
$ git pull --rebase https://github.com/rust-lang/rust.git master
$ git push --force-with-lease

The following commits are merge commits:

@mmvanheusden
Format std::env::consts docstrings
8c91a7e 
This clarifies possible outputs the constants might be.
@rustbot rustbot removed the has-merge-commits PR has merge commits, merge with caution. label Aug 15, 2024
@mmvanheusden
Copy link
Contributor Author

@rustbot review

@rustbot 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 Aug 15, 2024
workingjubilee
workingjubilee reviewed Aug 16, 2024
View reviewed changes
library/std/src/env.rs Outdated
Comment on lines 1044 to 1055
/// <details><summary>Full list of possible values</summary>
///
/// Some possible values:
/// * `".so"`
/// * `".dylib"`
/// * `".dll"`
/// * `".sgxs"`
/// * `".a"`
/// * `".elf"`
/// * `".wasm"`
/// * `""` (an empty string)
///
/// - .so
/// - .dylib
/// - .dll
/// </details>
Copy link
Member

Choose a reason for hiding this comment

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

this one should probably just mention that the list is the same as the list for DLL_EXTENSION

workingjubilee
workingjubilee reviewed Aug 16, 2024
View reviewed changes
library/std/src/env.rs Outdated
Comment on lines 983 to 986
/// * `"unix"`
/// * `"windows"`
/// * `"itron"`
/// * `""`
Copy link
Member

Choose a reason for hiding this comment

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

this is missing "wasm", see:

rust/compiler/rustc_target/src/spec/base/wasm.rs

Line 56 in 2c93fab

families: cvs!["wasm"],

which makes me slightly more uncomfortable about accepting this, because I am noticing how easily this will fall out of sync, yet it claims to be a "full list". hrm.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Yes, that is indeed tricky. Maybe I can add a notice that the list might be outdated but that kind of defeats the purpose of the list being there in the first place.

Copy link
Member

Choose a reason for hiding this comment

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

Hmm...

  • maybe we could factor out the lists and reinclude them via #[doc = include_str!("")]? I think we should prefer to autogenerate such lists and while demanding that of you seems silly, that will be easier to set up later if they are already smaller .md files.
  • I think saying "this may be outdated" is useless for the reason you mention, but we could address the temporality of the docs in another way. It could mention that this is the "current full list" inside the <summary></summary> tag. Or even say "the compiled list". :^) Or however you might want to phrase that? Even just between versions, the "actual" full list will grow over time, but docs reflect a snapshot in time.

workingjubilee
workingjubilee reviewed Aug 16, 2024
View reviewed changes
library/std/src/env.rs
/// - .nexe
/// - .pexe
/// - `""` (an empty string)
/// The possible values are identical to those of [`EXE_EXTENSION`], but with the leading period included.
Copy link
Member

Choose a reason for hiding this comment

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

Suggested change
/// The possible values are identical to those of [`EXE_EXTENSION`], but with the leading period included.
/// The possible values are identical to those of [`EXE_EXTENSION`], but with the leading period included
/// if `EXE_EXTENSION` is not `""`.

library/std/src/env.rs Outdated
Comment on lines 983 to 986
/// * `"unix"`
/// * `"windows"`
/// * `"itron"`
/// * `""`
Copy link
Member

Choose a reason for hiding this comment

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

Hmm...

  • maybe we could factor out the lists and reinclude them via #[doc = include_str!("")]? I think we should prefer to autogenerate such lists and while demanding that of you seems silly, that will be easier to set up later if they are already smaller .md files.
  • I think saying "this may be outdated" is useless for the reason you mention, but we could address the temporality of the docs in another way. It could mention that this is the "current full list" inside the <summary></summary> tag. Or even say "the compiled list". :^) Or however you might want to phrase that? Even just between versions, the "actual" full list will grow over time, but docs reflect a snapshot in time.

@workingjubilee
Copy link
Member

Hmm.

Better to ship this instead of holding it up further though, imo.

Thanks!

@bors r+ rollup

@bors
Copy link
Contributor

bors commented Sep 17, 2024

📌 Commit 0328c86 has been approved by workingjubilee

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Sep 17, 2024
@matthiaskrgr matthiaskrgr mentioned this pull request Sep 17, 2024
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Sep 17, 2024
@bors
Auto merge of rust-lang#130473 - matthiaskrgr:rollup-lf29374, r=matth…
f609b7e 
…iaskrgr

Rollup of 9 pull requests

Successful merges:

 - rust-lang#128535 (Format `std::env::consts` docstrings with markdown backticks)
 - rust-lang#128961 (Fix rust-lang#128930: Print documentation of CLI options missing their arg)
 - rust-lang#129988 (Use `Vec` in `rustc_interface::Config::locale_resources`)
 - rust-lang#130201 (Encode `coroutine_by_move_body_def_id` in crate metadata)
 - rust-lang#130275 (Don't call `extern_crate` when local crate name is the same as a dependency and we have a trait error)
 - rust-lang#130314 (Use the same precedence for all macro-like exprs)
 - rust-lang#130440 (Don't ICE in `opaque_hidden_inferred_bound` lint for RPITIT in trait with no default method body)
 - rust-lang#130458 (`rustc_codegen_ssa` cleanups)
 - rust-lang#130469 (Mark `where_clauses_object_safety` as removed)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 11fe22c into rust-lang:master Sep 17, 2024
6 checks passed
@rustbot rustbot added this to the 1.83.0 milestone Sep 17, 2024
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Sep 17, 2024
@rust-timer
Unrolled build for rust-lang#128535
16a41bf 
Rollup merge of rust-lang#128535 - mmvanheusden:master, r=workingjubilee

Format `std::env::consts` docstrings with markdown backticks

This clarifies possible outputs the constants might be.

**Before:**
--
<img src="https://github.com/user-attachments/assets/8ee8772a-7562-42a2-89be-f8772b76dbd5" width="500px">

**After:**
--
<img src="https://github.com/user-attachments/assets/4632e5e2-db3e-4372-b13e-006cc1701eb1" width="500px">
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@workingjubilee workingjubilee workingjubilee requested changes

Assignees

@workingjubilee workingjubilee

Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-libs Relevant to the library team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
1.83.0
Development

Successfully merging this pull request may close these issues.
4 participants
@mmvanheusden @rustbot @workingjubilee @bors