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

Ensure em dashes are recognizable in markup #11646

Merged
merged 5 commits into from
Feb 14, 2023
Merged

Conversation

Enyium
Copy link
Contributor

@Enyium Enyium commented Jan 28, 2023

An em dash (—) isn't well-recognizable in a monospace font. There were already a couple of locations where a hyphen was used instead. These were also corrected to —. — should reduce the probability of a hyphen being used for new entries.

@rustbot
Copy link
Collaborator

rustbot commented Jan 28, 2023

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

Please see the contribution instructions for more information.

@rustbot rustbot added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jan 28, 2023
Copy link
Member

@weihanglo weihanglo left a comment

Choose a reason for hiding this comment

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

Thanks for raising up this issue. I don't entirely agree with the solution, however. It would be better if we can keep the original source as readable as rendered one. Let's see what others say.

@Enyium
Copy link
Contributor Author

Enyium commented Jan 29, 2023

Just also saw it used here: https://github.com/dtolnay/indoc/blob/master/README.md?plain=1#L75.

@weihanglo
Copy link
Member

weihanglo commented Jan 30, 2023

As a non-English native speaker, choosing from different kinds of dashes is sometimes a headache to me. Yet, some developers prefer to read docs in their terminal emulators. I don't quite feel this patch more readable than the old one. The example you provided has the same problem:

image

Perhaps instead of this patch, we need a rule and an automation to help us fix this issue.

BTW, it seems like several crates from dtolnay pick HTML entities over literals. Curious to know the reason behind the back.

@weihanglo weihanglo added S-waiting-on-author Status: The marked PR is awaiting some action (such as code changes) from the PR author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jan 31, 2023
@Enyium
Copy link
Contributor Author

Enyium commented Feb 5, 2023

I'm not sure what is expected from me now. Your comment sounds like more opinions from your collegues are needed.

I can understand that — isn't great when reading the docs as raw markup. But in the case of Cargo docs, when is that really the case?

I don't know about @dtolnay's reasoning. When it comes to doc comments on code items, I'm torn between nice rendering and readability of raw markup, but ultimately favor the latter. Ideally, -- would render an em dash.

@ehuss
Copy link
Contributor

ehuss commented Feb 5, 2023

We can consider enabling smart punctuation (it's an option in book.toml), though it is a triple --- for emdash. I too would prefer to avoid using HTML entities, but I understand in most editors emdash isn't really obvious.

@Enyium
Copy link
Contributor Author

Enyium commented Feb 6, 2023

I noticed generated docs contain the typographically correct apostrophe where I wrote '. So, there's already something like that going on. Looks like a good basis to extend it with replacing -- with an en dash and --- with an em dash. I don't know what different systems are involved, though, considering this discussion was originally only about online Cargo docs. All docs should benefit from such a change. I already have a list in the following form in a doc comment of my yet unpublished code:

//! `function_1()` - Lorem ipsum dolor sit amet, consetetur sadipscing elitr, sed diam nonumy eirmod tempor invidunt ut labore et dolore magna aliquyam erat, sed diam voluptua.
//! `function_2()` - At vero eos et accusam et justo duo dolores et ea rebum.
//! `function_3()` - Stet clita kasd gubergren, no sea takimata sanctus est Lorem ipsum dolor sit amet.

@weihanglo
Copy link
Member

weihanglo commented Feb 6, 2023

They are two different systems AFAIK. The tool “The Cargo Book” uses is called mdbook, whereas rustdoc is a tool in https://github.com/rust-lang/rust/tree/master/src/librustdoc.

Eric has mentioned smart punctuations feature in mdbook, which is off by default. We could enable it here and then update each page accordingly. Do you want to give this approach a shot?

@Enyium
Copy link
Contributor Author

Enyium commented Feb 6, 2023

Eric has mentioned smart punctuations feature in mdbook, which is off by default. We could enable it here and then update each page accordingly. Do you want to give this approach a shot?

I could do that.


I just checked with cargo doc --open, and it already converts -- and ---, which is good news so far. So, I guess it also happens for docs.rs. But why does the rendered documentation VS Code with rust-analyzer shows when hovering over items still display -- and --- with gaps?

@weihanglo
Copy link
Member

I just checked with cargo doc --open, and it already converts -- and ---, which is good news so far. So, I guess it also happens for docs.rs. But why does the rendered documentation VS Code with rust-analyzer shows when hovering over items still display -- and --- with gaps?

I guess rust-analyzer provides just raw contents to the LSP client, which is effectively VS Code. The display depends on how a client renders them. rust-analyzer indeed can provide unicode version of them, yet I don't feel like rendering should be a thing done by a language server.

@Enyium
Copy link
Contributor Author

Enyium commented Feb 6, 2023

I don't feel like rendering should be a thing done by a language server.

And just the smart-punctuation prestage? Is it possible on it's own, while otherwise still serving markdown to the IDE? Since it's an addition to markdown, we may not ever see it rendered better.

@weihanglo
Copy link
Member

… while otherwise still serving markdown to the IDE? …

Let's focus on the PR itself. If you want a further discussion, feel free to chat on the dedicated rust-analyzer zulip stream.

@Enyium Enyium closed this Feb 7, 2023
@Enyium Enyium deleted the patch-1 branch February 7, 2023 00:29
@Enyium Enyium restored the patch-1 branch February 7, 2023 00:30
@Enyium
Copy link
Contributor Author

Enyium commented Feb 7, 2023

Sorry for the noise. Seems like renaming a branch closes the PR.

@Enyium Enyium reopened this Feb 7, 2023
@Enyium Enyium changed the title Change literal em dashes to — Ensure em dashes are recognizable in markup Feb 7, 2023
@Enyium
Copy link
Contributor Author

Enyium commented Feb 7, 2023

I carefully changed .md files in src/doc. Among the the source texts that I changed to --- were hyphens (Markdown list dashes unchanged), double-hyphens (command line switches and switch separators unchanged) and em dashes.

@weihanglo weihanglo added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-author Status: The marked PR is awaiting some action (such as code changes) from the PR author. labels Feb 8, 2023
src/doc/book.toml Show resolved Hide resolved
src/doc/contrib/src/architecture/codebase.md Show resolved Hide resolved
src/doc/man/cargo-bench.md Show resolved Hide resolved
Copy link
Member

@weihanglo weihanglo left a comment

Choose a reason for hiding this comment

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

Thanks for your quick reply!

One thing I forgot to mention. We also need to enable output.html.curly-quotes in src/doc/contrib/book.toml.

@Enyium
Copy link
Contributor Author

Enyium commented Feb 10, 2023

May I add a translation of src/doc/build-man.sh to a Windows batch script?

@weihanglo
Copy link
Member

May I add a translation of src/doc/build-man.sh to a Windows batch script?

I don't have enough knowledge for reviewing Windows batch script. I might redirect it to another reviewer for such a change.

@Enyium
Copy link
Contributor Author

Enyium commented Feb 14, 2023

  • Rebased my commits to include respective test changes in commit that makes it break. Include cargo fmt changes in suitable commit.
  • Rebased branch onto latest commit from master and resolved conflicts.

Copy link
Member

@weihanglo weihanglo left a comment

Choose a reason for hiding this comment

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

Yeah we eventually got here! Thank you for your contribution and patience so far!

This PR changes the help manual rendering, including

  • Enabled smart punctuation in mdbook for The Cargo book and Cargo Contributor Guide.
  • Upgraded pulldown-cmark to v0.9.2. This upgrade breaks some tests, but they are indeed the correct behavior (see Ensure em dashes are recognizable in markup #11646 (comment)).
  • Implemented the same smart punctuation functionality in mdman for roff and text output. This changes the rendered result of OptionsHelper in mdman. It renders
    {{#option "_named-arg..._"}}
    A named argument.
    {{/option}}
    to an <dt> containing an id with unicode ellipsis option-options-named-arg…, which previously was just three dots option-options-named-arg.... I scanned the codebase and it seems that in Cargo we don't have such an option, so it is unlikely to break old links. (See Ensure em dashes are recognizable in markup #11646 (comment))

After this PR, when writing contents for The Cargo book or Cargo Contributor Guide, use --- for em dash. That's the whole point. If anyone feels wrong to merge this, let us know here or on Zulip. We can always consider a revert.

@weihanglo
Copy link
Member

@bors r+

@bors
Copy link
Contributor

bors commented Feb 14, 2023

📌 Commit b1754b2 has been approved by weihanglo

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 Feb 14, 2023
@bors
Copy link
Contributor

bors commented Feb 14, 2023

⌛ Testing commit b1754b2 with merge 4262636...

@bors
Copy link
Contributor

bors commented Feb 14, 2023

☀️ Test successful - checks-actions
Approved by: weihanglo
Pushing 4262636 to master...

@bors bors merged commit 4262636 into rust-lang:master Feb 14, 2023
@Enyium Enyium mentioned this pull request Feb 15, 2023
bors added a commit that referenced this pull request Feb 15, 2023
Amend `mdman` tests.

- Revert most of changes to expected test output from commit 2a4ec9f.
- Keep later changes to expected test output from commit 0263ef4.
- Change test input that's converted to trigger similar output as previously.

Following [this](#11646 (comment)) comment from `@ehuss.`
weihanglo added a commit to weihanglo/cargo that referenced this pull request Feb 15, 2023
Previously mdbook was bumped in rust-lang#11646 for contrib.yml worflow
but forgot main.yaml workflow. This makes the two in sync and also
upgrade to the latest 0.4.27.
(Though there is nothing really changed for application users as us)
weihanglo added a commit to weihanglo/cargo that referenced this pull request Feb 15, 2023
Previously mdbook was bumped in rust-lang#11646 for contrib.yml worflow
but main.yaml workflow. This makes the two in sync and also
upgrades to the latest 0.4.27. (Though there is nothing really
changed for application users as us)
bors added a commit that referenced this pull request Feb 15, 2023
chore: bump mdbook to 0.4.27

Previously mdbook was bumped in #11646 for contrib.yml worflow
but main.yaml workflow. This makes the two in sync and also
upgrades to the latest 0.4.27. (Though there is nothing really
changed for application users as us)
matthiaskrgr pushed a commit to matthiaskrgr/rust that referenced this pull request Feb 18, 2023
10 commits in 39c13e67a5962466cc7253d41bc1099bbcb224c3..17b3d0de0897e1c6b8ca347bd39f850bb0a5b9f6
2023-02-12 02:01:08 +0000 to 2023-02-17 19:45:09 +0000

- fix: unsupported protocol error on old macos version (rust-lang/cargo#11733)
- Error on invalid alphanumeric token for crates.io (rust-lang/cargo#11600)
- Add clippy lints (rust-lang/cargo#11722)
- chore: Make dependencies alphabetical order (rust-lang/cargo#11719)
- chore: bump mdbook to 0.4.27 (rust-lang/cargo#11716)
- Amend `mdman` tests. (rust-lang/cargo#11715)
- Run CI for macOS on nightly (rust-lang/cargo#11712)
- doc: doc comments and intra-doc links for `core::compiler` (rust-lang/cargo#11711)
- Ensure em dashes are recognizable in markup (rust-lang/cargo#11646)
- Set CARGO_BIN_NAME environment variable also for binary examples (rust-lang/cargo#11705)
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Feb 18, 2023
Update cargo

10 commits in 39c13e67a5962466cc7253d41bc1099bbcb224c3..17b3d0de0897e1c6b8ca347bd39f850bb0a5b9f6 2023-02-12 02:01:08 +0000 to 2023-02-17 19:45:09 +0000

- fix: unsupported protocol error on old macos version (rust-lang/cargo#11733)
- Error on invalid alphanumeric token for crates.io (rust-lang/cargo#11600)
- Add clippy lints (rust-lang/cargo#11722)
- chore: Make dependencies alphabetical order (rust-lang/cargo#11719)
- chore: bump mdbook to 0.4.27 (rust-lang/cargo#11716)
- Amend `mdman` tests. (rust-lang/cargo#11715)
- Run CI for macOS on nightly (rust-lang/cargo#11712)
- doc: doc comments and intra-doc links for `core::compiler` (rust-lang/cargo#11711)
- Ensure em dashes are recognizable in markup (rust-lang/cargo#11646)
- Set CARGO_BIN_NAME environment variable also for binary examples (rust-lang/cargo#11705)

r? `@ghost`
@ehuss ehuss added this to the 1.69.0 milestone Mar 9, 2023
@Enyium Enyium deleted the patch-1 branch October 8, 2024 11:22
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-cli-help Area: built-in command-line help A-documenting-cargo-itself Area: Cargo's documentation S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants