Skip to content

librustdoc: improve testnames for doctests #29600

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

Merged
merged 1 commit into from
Nov 6, 2015

Conversation

Ryman
Copy link
Contributor

@Ryman Ryman commented Nov 5, 2015

Old doctest names

test sync::atomic::load_0 ... ok
test sync::atomic::load_0 ... ok
test sync::atomic::load_0 ... ok
test sync::atomic::load_0 ... ok

New doctest names

test sync::atomic::AtomicBool::load_0 ... ok
test sync::atomic::AtomicIsize::load_0 ... ok
test sync::atomic::AtomicPtr<T>::load_0 ... ok
test sync::atomic::AtomicUsize::load_0 ... ok

@rust-highfive
Copy link
Contributor

r? @alexcrichton

(rust_highfive has picked a reviewer for you, use r? to override)

.next()
.or_else(|| typename_if_impl(&item));

let pushed = name.map(|name| self.names.push(name)).is_some();
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 a little bit hard to follow, so perhaps this could use explicit control flow + if let Some(...) = ... to match what's going on here? Currently it's somewhat non-idiomatic to call .iter() on an option or rely on functions like map for side effects.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Heh, it got a bit out of hand, will do.

@Ryman Ryman force-pushed the rustdoc_better_testnames branch from 57a0430 to 26e99fc Compare November 5, 2015 19:24
@Ryman
Copy link
Contributor Author

Ryman commented Nov 5, 2015

@alexcrichton Adjusted from feedback


fn typename_if_impl(item: &clean::Item) -> Option<String> {
if let clean::ItemEnum::ImplItem(ref impl_) = item.inner {
let path = <clean::Type as ToString>::to_string(&impl_.for_);
Copy link
Member

Choose a reason for hiding this comment

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

Ah yeah it's fine to use .to_string() here, I was just getting confused :(

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Just pushed adding.to_string() again :)

@Ryman Ryman force-pushed the rustdoc_better_testnames branch from 26e99fc to 4202615 Compare November 6, 2015 00:37
@alexcrichton
Copy link
Member

@bors: r+ 4202615

bors added a commit that referenced this pull request Nov 6, 2015
Old doctest names
```bash
test sync::atomic::load_0 ... ok
test sync::atomic::load_0 ... ok
test sync::atomic::load_0 ... ok
test sync::atomic::load_0 ... ok
```

New doctest names
```bash
test sync::atomic::AtomicBool::load_0 ... ok
test sync::atomic::AtomicIsize::load_0 ... ok
test sync::atomic::AtomicPtr<T>::load_0 ... ok
test sync::atomic::AtomicUsize::load_0 ... ok
```
@bors
Copy link
Collaborator

bors commented Nov 6, 2015

⌛ Testing commit 4202615 with merge 02d9f29...

@bors bors merged commit 4202615 into rust-lang:master Nov 6, 2015
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants