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

Remove weird handling of ## in code examples #118825

Closed

Conversation

GuillaumeGomez
Copy link
Member

@GuillaumeGomez GuillaumeGomez commented Dec 11, 2023

As mentioned in #118711, the handling of ## in code blocks in code examples is very surprising. Let's see if crates are using it with a crater run.

description

Currently, if you have two # characters following each other, the first one will be stripped. So this code:

/// ```
/// ## something
/// ```

is rendered as:

# something

This PR removes this behaviour and won't strip the first # item so the previous code will be rendered:

## something

Motivation

It looks more like a bug rather than a feature as it is a very unexpected behaviour. This PR intends to remove it. As seen in the crater run below, it seems no published crate relies on it either.

r? @notriddle

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. labels Dec 11, 2023
@GuillaumeGomez
Copy link
Member Author

GuillaumeGomez commented Dec 11, 2023

Let's start a crater run then.

@craterbot run mode=build-and-test +cargoflags=--doc

@craterbot
Copy link
Collaborator

🚨 Error: failed to parse the command

🆘 If you have any trouble with Crater please ping @rust-lang/infra!
ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

@GuillaumeGomez
Copy link
Member Author

Seems like what I wanted to do is actually not possible...

@craterbot run mode=build-and-test

@craterbot
Copy link
Collaborator

🚨 Error: missing start toolchain

🆘 If you have any trouble with Crater please ping @rust-lang/infra!
ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

@GuillaumeGomez
Copy link
Member Author

@craterbot run mode=build-and-test

@craterbot
Copy link
Collaborator

🚨 Error: missing start toolchain

🆘 If you have any trouble with Crater please ping @rust-lang/infra!
ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

@GuillaumeGomez
Copy link
Member Author

Ah, maybe I need to have run try first...

@bors try

@rust-log-analyzer

This comment has been minimized.

@bors
Copy link
Contributor

bors commented Dec 11, 2023

⌛ Trying commit 289eeac with merge 26fc5aa...

bors added a commit to rust-lang-ci/rust that referenced this pull request Dec 11, 2023
… r=<try>

Remove weird handling of ## in code examples

As mentioned in rust-lang#118711, the handling of `##` in code blocks in code examples is very surprising. Let's see if crates are using it with a crater run.

r? `@notriddle`
@GuillaumeGomez
Copy link
Member Author

@bors try-

@bors try

@bors
Copy link
Contributor

bors commented Dec 11, 2023

⌛ Trying commit 8b60110 with merge 5e42734...

bors added a commit to rust-lang-ci/rust that referenced this pull request Dec 11, 2023
… r=<try>

Remove weird handling of ## in code examples

As mentioned in rust-lang#118711, the handling of `##` in code blocks in code examples is very surprising. Let's see if crates are using it with a crater run.

r? `@notriddle`
@bors
Copy link
Contributor

bors commented Dec 11, 2023

☀️ Try build successful - checks-actions
Build commit: 5e42734 (5e4273472e1b2f223618a9b3def795f6224f241d)

@GuillaumeGomez
Copy link
Member Author

@craterbot run mode=build-and-test

@craterbot
Copy link
Collaborator

👌 Experiment pr-118825 created and queued.
🤖 Automatically detected try build 5e42734
🔍 You can check out the queue and this experiment's details.

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

@craterbot craterbot added S-waiting-on-crater Status: Waiting on a crater run to be completed. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Dec 11, 2023
@craterbot
Copy link
Collaborator

🚧 Experiment pr-118825 is now running

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

@craterbot
Copy link
Collaborator

🎉 Experiment pr-118825 is completed!
📊 138 regressed and 107 fixed (398750 total)
📰 Open the full report.

⚠️ If you notice any spurious failure please add them to the blacklist!
ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

@craterbot craterbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-crater Status: Waiting on a crater run to be completed. labels Dec 15, 2023
@GuillaumeGomez
Copy link
Member Author

Seems like there is no change as far as I can see?

@GuillaumeGomez
Copy link
Member Author

Might be worth starting an FCP then since it's a breaking change.

@rfcbot fcp merge

@rfcbot
Copy link

rfcbot commented Dec 17, 2023

Team member @GuillaumeGomez has proposed to merge this. The next step is review by the rest of the tagged team members:

No concerns currently listed.

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

@rfcbot rfcbot added proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. labels Dec 17, 2023
@Nemo157
Copy link
Member

Nemo157 commented Dec 17, 2023

Seems like there is no change as far as I can see?

I see a broken doctest on the second "test failed (unknown)" I looked at: https://crater-reports.s3.amazonaws.com/pr-118825/try%235e4273472e1b2f223618a9b3def795f6224f241d/reg/ctflag-0.1.2/log.txt

@GuillaumeGomez
Copy link
Member Author

Good catch! So we can't revert it like this apparently...

@Manishearth
Copy link
Member

Hold on, is the change to always render ##? That doesn't seem quite right to me either.

Here's what I expect:

  • Unindented # becomes hidden
  • Indented # is never hidden
  • #! and #[ are special cased for this, and do not hide. If you want to hide an attribute, use ##![...]

@notriddle
Copy link
Contributor

Here's what I expect:

  • Unindented # becomes hidden
  • Indented # is never hidden
  • #! and #[ are special cased for this, and do not hide. If you want to hide an attribute, use ##![...]

I would prefer to avoid adding more indentation-based logic. This kind of thing makes it hard to tell if something is considered indented or not.

1d19462

https://talk.commonmark.org/t/unsure-how-the-0-29-spec-handles-this-list-indentation/3326

@Manishearth
Copy link
Member

This isn't about list indentation, though, this is about codeblock indentation; which tends to just follow the indentation of the opening backticks. I think that's an okay heuristic.

Either way, I don't see this PR as an improvement; it seems to not be fixing the problem brought up in #118711 but instead trying a new direction and I can't see the reasoning behind that.

@GuillaumeGomez
Copy link
Member Author

Then I misunderstood what I thought the problem was: I thought the issue was that we were replacing ## with #.

@Manishearth
Copy link
Member

I thought that was a way of doing an escape code

@GuillaumeGomez
Copy link
Member Author

An escape for # for sure but that's the only case.

@Manishearth
Copy link
Member

Right. I think that's an intentional design to escape for #

@Nemo157
Copy link
Member

Nemo157 commented Dec 19, 2023

What seems like a bug to me is that we do this for any ##, while what needs escaping is # (with a trailing space), so we only needed to replace ## (with a trailing space), not e.g. ##[. Unfortunately code has come to rely on the existing buggy behavior, so likely the only way to fix it is as an edition change.

@Manishearth
Copy link
Member

Oh! So # with no trailing space isn't treated as a hide marker? That seems fine then.

But yeah we shouldn't be doing this for every ##.

I think someone needs to properly write down what the current behavior is and what it would be in an ideal world, because so far nobody has given a full rundown of the situation.

@notriddle
Copy link
Contributor

I also found the issue where this was originally added:

#41783

@Dylan-DPC
Copy link
Member

Had a discussion with the author and they did mention that this pr is headed nowhere so the best option is to close it.

@Dylan-DPC Dylan-DPC closed this Apr 18, 2024
@rfcbot rfcbot removed proposed-final-comment-period Proposed to merge/close by relevant subteam, see T-<team> label. Will enter FCP once signed off. disposition-merge This issue / PR is in PFCP or FCP with a disposition to merge it. labels Apr 18, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

10 participants