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

Remove whitespaces in doctests' name #89422

Merged
merged 1 commit into from
Oct 2, 2021
Merged

Remove whitespaces in doctests' name #89422

merged 1 commit into from
Oct 2, 2021

Conversation

GuillaumeGomez
Copy link
Member

@GuillaumeGomez GuillaumeGomez commented Oct 1, 2021 •
edited
Loading

Fixes #88263.

Instead of handling white spaces when we filter tests (which would be quite complicated since we split on them!), I propose to instead replace them with dashes.

So for example, this:

test foo.rs - Iter2<T, P>::len (line 13) ... ok
test foo.rs - Iter<T, P>::len (line 4) ... ok

becomes:

test foo.rs - Iter<T,-P>::len (line 4) ... ok
test foo.rs - Iter2<T,-P>::len (line 13) ... ok

r? @jyn514

@GuillaumeGomez GuillaumeGomez added T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. A-doctests Area: Documentation tests, run by rustdoc labels Oct 1, 2021
@rust-highfive
Copy link
Collaborator

r? @CraftSpider

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Oct 1, 2021
@GuillaumeGomez
Copy link
Member Author

Failed the assignment...

r? @jyn514

@rust-highfive rust-highfive assigned jyn514 and unassigned CraftSpider Oct 1, 2021
@Urgau
Copy link
Member

Urgau commented Oct 1, 2021

Is it not possible to use a no-break space « » (ie U+00A0) instead of « - » ?
This would have the benefits of keeping the « same » visual appearance.

@GuillaumeGomez
Copy link
Member Author

If we do that, it's very likely that people will simply enter the name (with normal white space) and end up in the original issue.

@jyn514
Copy link
Member

jyn514 commented Oct 1, 2021

r? @CraftSpider

I don't have time to review this.

@rust-highfive rust-highfive assigned CraftSpider and unassigned jyn514 Oct 1, 2021
@CraftSpider
Copy link
Contributor

I'm not sure the look with dashes is great, but I also agree that using no-break spaces is likely to lead people to the same issue.

Possible Alternative: How often do spaces show up, and where? If we're pretty sure spaces won't show up between identifiers, would it make sense to just strip them entirely? So we instead end up with Iter<T,P>::len, which while imperfect I think still looks a little better, and solves the issue.

@GuillaumeGomez
Copy link
Member Author

Great idea! I prefer this alternative much more! I'll update it then.

@GuillaumeGomez GuillaumeGomez force-pushed the doctest-whitespace-name branch from 1690887 to 3792be6 Compare October 1, 2021 19:24
@GuillaumeGomez
Copy link
Member Author

Done!

@CraftSpider
Copy link
Contributor

Awesome, looks good to me
@bors r+

@bors
Copy link
Contributor

bors commented Oct 1, 2021

📌 Commit 3792be6 has been approved by CraftSpider

@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 Oct 1, 2021
@GuillaumeGomez
Copy link
Member Author

@bors rollup

@Manishearth Manishearth mentioned this pull request Oct 1, 2021
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Oct 1, 2021
@bors
Auto merge of rust-lang#89449 - Manishearth:rollup-3alb61f, r=Manishe…
6e12110 
…arth

Rollup of 7 pull requests

Successful merges:

 - rust-lang#85223 (rustdoc: Clarified the attribute which prompts the warning)
 - rust-lang#88847 (platform-support.md: correct ARMv7+MUSL platform triple notes)
 - rust-lang#88963 (Coerce const FnDefs to implement const Fn traits )
 - rust-lang#89376 (Fix use after drop in self-profile with llvm events)
 - rust-lang#89422 (Replace whitespaces in doctests' name with dashes)
 - rust-lang#89440 (Clarify a sentence in the documentation of Vec (rust-lang#84488))
 - rust-lang#89441 (Normalize after substituting via `field.ty()`)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 04ba153 into rust-lang:master Oct 2, 2021
@rustbot rustbot added this to the 1.57.0 milestone Oct 2, 2021
@GuillaumeGomez GuillaumeGomez deleted the doctest-whitespace-name branch October 2, 2021 09:35
@GuillaumeGomez GuillaumeGomez changed the title Replace whitespaces in doctests' name with dashes Remove whitespaces in doctests' name Oct 2, 2021
flip1995 pushed a commit to flip1995/rust that referenced this pull request Oct 7, 2021
@bors
Auto merge of rust-lang#89449 - Manishearth:rollup-3alb61f, r=Manishe…
2cb37a1 
…arth

Rollup of 7 pull requests

Successful merges:

 - rust-lang#85223 (rustdoc: Clarified the attribute which prompts the warning)
 - rust-lang#88847 (platform-support.md: correct ARMv7+MUSL platform triple notes)
 - rust-lang#88963 (Coerce const FnDefs to implement const Fn traits )
 - rust-lang#89376 (Fix use after drop in self-profile with llvm events)
 - rust-lang#89422 (Replace whitespaces in doctests' name with dashes)
 - rust-lang#89440 (Clarify a sentence in the documentation of Vec (rust-lang#84488))
 - rust-lang#89441 (Normalize after substituting via `field.ty()`)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@lowr lowr mentioned this pull request Aug 1, 2022
Merged
bors added a commit to rust-lang/rust-analyzer that referenced this pull request Aug 1, 2022
@bors
Auto merge of #12918 - lowr:fix/doctest-names, r=Veykril
5edbdd1 
fix: remove whitespaces from doctest names

When rustdoc runs doctests, it removes whitespaces from the tests' path ([code](https://github.com/rust-lang/rust/blob/25bb1c13bd472b75ceebee3b8dcf4dcbc431a8be/src/librustdoc/doctest.rs#L951)). See rust-lang/rust#89422 for details.

Interestingly enough, "Run doctest" has been working without much problem even though rust-analyzer hasn't followed the change. This is because cargo passes the test name to rustdoc via `--test-args` option, and then rustdoc [splits it by whitespace](https://github.com/rust-lang/rust/blob/25bb1c13bd472b75ceebee3b8dcf4dcbc431a8be/src/librustdoc/config.rs#L513-L514); the last element of the split test name **always** matches the test name that rustdoc generates.

However, it may run other tests unexpectedly (to be precise, this has long since been a thing because of the split). Consider the following example:

```rust
struct A<T, U>(T, U);
struct B<T, U>(T, U);
/// ```
/// doctest here
/// ```
impl<T, U> A<T, U> {}
/// ```
/// doctest here
/// ```
impl<T, U> B<T, U> {}
```

When you "Run doctest" either of the two, rustdoc considers "U>" one of the test specs and both doctests are run. This patch fixes it by following rustdoc and removing the whitespace from the doctests' name.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers
No reviews
Assignees

@CraftSpider CraftSpider

Labels
A-doctests Area: Documentation tests, run by rustdoc 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
Milestone
1.57.0
Development

Successfully merging this pull request may close these issues.

rustdoc does not filter correctly doctests when their name contain spaces.
7 participants
@GuillaumeGomez @rust-highfive @Urgau @jyn514 @CraftSpider @bors @rustbot