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

Fix unindent in doc comments #78400

Merged
merged 5 commits into from
Nov 3, 2020
Merged

Conversation

GuillaumeGomez
Copy link
Member

Fixes #70732

r? @jyn514

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Oct 26, 2020
@Nemo157
Copy link
Member

Nemo157 commented Oct 26, 2020

How come this doesn't break https://github.com/rust-lang/rust/blob/master/src/test/rustdoc/issue-42760.rs, which was explicitly testing for the old behaviour?

@GuillaumeGomez
Copy link
Member Author

This test only check that there is a title, not that the indent isn't broken as far as I can tell?

src/librustdoc/passes/unindent_comments.rs Outdated Show resolved Hide resolved
src/librustdoc/passes/unindent_comments.rs Outdated Show resolved Hide resolved
src/test/rustdoc-ui/intra-links-warning.stderr Outdated Show resolved Hide resolved
src/librustdoc/passes/unindent_comments.rs Outdated Show resolved Hide resolved
@jyn514 jyn514 added the T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. label Oct 26, 2020
@Nemo157
Copy link
Member

Nemo157 commented Oct 26, 2020

If the unindent pass doesn't strip the leading space (because of the #[doc] attribute without it), then the # Example heading shouldn't be parsed as a heading because of the leading space. Skimming the code now I wonder if it's because of the special casing of the first line of a fragment, but I can't really follow it to tell what it's trying to do.

@GuillaumeGomez
Copy link
Member Author

@Nemo157 Well, for me it seems like it should work because sugared doc comments should be handled a bit differently than the others (they need to have one extra whitespace after all).

@jyn514 Thanks to your remarks, I greatly improved the code. :)

src/librustdoc/passes/unindent_comments.rs Outdated Show resolved Hide resolved
src/librustdoc/passes/unindent_comments.rs Outdated Show resolved Hide resolved
@GuillaumeGomez GuillaumeGomez force-pushed the fix-unindent branch 2 times, most recently from 336bab3 to 13b3187 Compare October 26, 2020 15:10
@Nemo157
Copy link
Member

Nemo157 commented Oct 26, 2020

@Nemo157 Well, for me it seems like it should work because sugared doc comments should be handled a bit differently than the others (they need to have one extra whitespace after all).

But they don't, you can have

///# Documentation
///
///Without leading whitespace

just fine, it's just convention to have a space between the doc-comment marker and the docs.

@GuillaumeGomez
Copy link
Member Author

Well, glad I treated that correctly then. Thanks for the extra information! :)

@GuillaumeGomez
Copy link
Member Author

Hum, actually I might need to make a small update in the code. :3

src/librustdoc/passes/unindent_comments/tests.rs Outdated Show resolved Hide resolved
src/test/rustdoc/unindent.rs Outdated Show resolved Hide resolved
@jyn514
Copy link
Member

jyn514 commented Oct 28, 2020

We're discussing this on discord, but either way the following test cases we came up with should be added:

///no whitespace
#[doc = " lol"]
///    4 whitespaces!
#[doc = "something"]

(https://github.com/clap-rs/clap/blob/625c201f657f091d0eb627c84db3e68eec56d507/src/build/arg/mod.rs#L1901)

    #[cfg_attr(not(unix), doc = " ```ignore")]
    #[cfg_attr(unix, doc = " ```rust")]
    /// fn has_ampersand(v: &OsStr) -> Result<(), String> {
    ///     if v.as_bytes().iter().any(|b| *b == b'&') { return Ok(()); }
    ///     Err(String::from("The value did not contain the required & sigil"))
    /// }

Additionally, note that the fixed version of #70732 in the test case uses 4 spaces instead of 5, which differs from the original bug report. IMO this seems reasonable to match #42760.

There should also be test cases for doc(include = ...).

@GuillaumeGomez
Copy link
Member Author

So I changed a bit how the unindent is handled. For example:

#[doc = "hello
         second"]

Before, we didn't take into account the first line that in this case, we could align the doc based on the next lines indent. It was very misleading and not that great as I discovered while updating this PR. I always assumed it was supposed to be:

#[doc = "hello
second"]

I definitely think it's better this way.

Apart from that change, I also added comments and extended tests to add doc includes.

@bors
Copy link
Contributor

bors commented Nov 1, 2020

📌 Commit 87f2897 has been approved by jyn514

@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 Nov 1, 2020
@bors
Copy link
Contributor

bors commented Nov 1, 2020

⌛ Testing commit 87f2897 with merge 1d5057d91ad6ad54b899a9b93b5b6b8f1fe73ffa...

@bors
Copy link
Contributor

bors commented Nov 1, 2020

💔 Test failed - checks-actions

@bors bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Nov 1, 2020
@GuillaumeGomez
Copy link
Member Author

Fetch error, let's retry!

@bors: retry

@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 Nov 1, 2020
@bors
Copy link
Contributor

bors commented Nov 1, 2020

⌛ Testing commit 87f2897 with merge 83a3d46759cbc915445e3f07addb85a82085b2c7...

@jyn514
Copy link
Member

jyn514 commented Nov 1, 2020

If this fails because of another zlib error, let's avoid retrying, @ehuss is working on a fix for the underlying issue in https://rust-lang.zulipchat.com/#narrow/stream/246057-t-cargo/topic/pinging.20for.20zlib.20error.

@bors
Copy link
Contributor

bors commented Nov 1, 2020

💔 Test failed - checks-actions

@bors bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Nov 1, 2020
@GuillaumeGomez
Copy link
Member Author

Seeing that other PRs seem to pass, let's give it another try.

@bors: retry

@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 Nov 2, 2020
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this pull request Nov 3, 2020
bors added a commit to rust-lang-ci/rust that referenced this pull request Nov 3, 2020
Rollup of 8 pull requests

Successful merges:

 - rust-lang#78376 (Treat trailing semicolon as a statement in macro call)
 - rust-lang#78400 (Fix unindent in doc comments)
 - rust-lang#78575 (Add a test for compiletest rustc-env & unset-rustc-env directives)
 - rust-lang#78616 (Document -Zinstrument-coverage)
 - rust-lang#78663 (Fix ICE when a future-incompat-report has its command-line level capped)
 - rust-lang#78664 (Fix intrinsic size_of stable link)
 - rust-lang#78668 (inliner: Remove redundant loop)
 - rust-lang#78676 (add mipsel-unknown-none target)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit e731a5a into rust-lang:master Nov 3, 2020
@rustbot rustbot added this to the 1.49.0 milestone Nov 3, 2020
@GuillaumeGomez GuillaumeGomez deleted the fix-unindent branch November 3, 2020 09:53
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. 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.

doc attribute removes whitespace
7 participants