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

rustdoc: Sort implementors #53941

Merged
merged 3 commits into from
Sep 16, 2018
Merged

rustdoc: Sort implementors #53941

merged 3 commits into from
Sep 16, 2018

Conversation

kzys
Copy link
Contributor

@kzys kzys commented Sep 4, 2018

Fixes #53812

@rust-highfive
Copy link
Collaborator

r? @steveklabnik

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

@rust-highfive

This comment has been minimized.

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Sep 4, 2018
@kzys
Copy link
Contributor Author

kzys commented Sep 4, 2018

Sorry, it shouldn't have the submodules. The latest commit doesn't have them.

@kzys
Copy link
Contributor Author

kzys commented Sep 5, 2018

@GuillaumeGomez
Copy link
Member

Thanks!

The lack of test is problematic though, but you can't really add one since the UI tests are still pending... Should we wait the merge of the UI tests PR @QuietMisdreavus?

@kzys
Copy link
Contributor Author

kzys commented Sep 5, 2018

I'm fine with waiting #53319.

@TimNN
Copy link
Contributor

TimNN commented Sep 11, 2018

Ping from triage @GuillaumeGomez: It looks like we won't be able to run the UI tests on CI soon. Do you still want to mark this PR as blocked on it? Otherwise, this PR requires your review.

@GuillaumeGomez
Copy link
Member

This is really problematic... Code seems good but we have no way to ensure that it won't break later in the future... If everyone's fine with that, then r=me.

@bors
Copy link
Contributor

bors commented Sep 12, 2018

☔ The latest upstream changes (presumably #54146) made this pull request unmergeable. Please resolve the merge conflicts.

@kzys
Copy link
Contributor Author

kzys commented Sep 13, 2018

How about adding tests under src/test/rustdoc/? Seems like I can validate something like "the first element under this element must be X".

@GuillaumeGomez
Copy link
Member

Let's go for this then...

@QuietMisdreavus
Copy link
Member

Testing something like "the first element in this list must be X" should fit into a src/test/rustdoc test, since the XPath implementation we use should be able to support that:

* `@has PATH XPATH PATTERN` and `@matches PATH XPATH PATTERN` checks for
the presence of given `XPATH` in the given HTML file, and also
the occurrence of given `PATTERN` in the matching node or attribute.
Only one occurrence of given pattern in the match is enough.
`PATH` should be a valid and well-formed HTML file. It does *not*
accept arbitrary HTML5; it should have matching open and close tags
and correct entity references at least.
`XPATH` is an XPath expression to match. This is fairly limited:
`tag`, `*`, `.`, `//`, `..`, `[@attr]`, `[@attr='value']`, `[tag]`,
`[POS]` (element located in given `POS`), `[last()-POS]`, `text()`
and `@attr` (both as the last segment) are supported. Some examples:
- `//pre` or `.//pre` matches any element with a name `pre`.
- `//a[@href]` matches any element with an `href` attribute.
- `//*[@class="impl"]//code` matches any element with a name `code`,
which is an ancestor of some element which `class` attr is `impl`.
- `//h1[@class="fqn"]/span[1]/a[last()]/@class` matches a value of
`class` attribute in the last `a` element (can be followed by more
elements that are not `a`) inside the first `span` in the `h1` with
a class of `fqn`. Note that there cannot be no additional elements
between them due to the use of `/` instead of `//`.
Do not try to use non-absolute paths, it won't work due to the flawed
ElementTree implementation. The script rejects them.
For the text matches (i.e. paths not ending with `@attr`), any
subelements are flattened into one string; this is handy for ignoring
highlights for example. If you want to simply check the presence of
given node or attribute, use an empty string (`""`) as a `PATTERN`.

I'd be okay with a test that did that.

@GuillaumeGomez
Copy link
Member

With such a test, it'd be as good as a UI one. Thanks for the head up @QuietMisdreavus!

@kzys
Copy link
Contributor Author

kzys commented Sep 14, 2018

Hmm, is there a way to have the test file as std?

pub trait MyIterator {
}

macro_rules! array_impls {
    ($($N:expr)+) => {
        $(
            impl<'a, T> MyIterator for &'a mut [T; $N] {
            }
        )+
    }
}

array_impls! { 0 1 2 }

The above is built as its own crate, which makes all MyIterator implementations as Implementations on Foreign Types. And the section somehow doesn't have this problem. In other words, I can't repro the issue in this test.

@QuietMisdreavus
Copy link
Member

Try using a wrapper type instead of plain arrays:

pub trait MyIterator {
}

pub struct MyStruct<T>(T);

macro_rules! array_impls {
    ($($N:expr)+) => {
        $(
            impl<'a, T> MyIterator for MyStruct<&'a mut [T; $N]> {
            }
        )+
    }
}

array_impls! { 0 1 2 }

That should make rustdoc treat the implementation as on a local type.

The way it defines implementations is unrealistic though.
}
}

// @has issue_53812/trait.MyIterator.html '//*[@id="implementors-list"]//h3[1]' 'MyStruct<[T; 0]>'
Copy link
Member

Choose a reason for hiding this comment

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

Please check h3[2], h3[3] and h3[4] too. Like that we're sure it's not just luck.

@GuillaumeGomez
Copy link
Member

Thanks!

@bors: r+ rollup

@bors
Copy link
Contributor

bors commented Sep 16, 2018

📌 Commit 2fe4503 has been approved by GuillaumeGomez

@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 Sep 16, 2018
GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this pull request Sep 16, 2018
bors added a commit that referenced this pull request Sep 16, 2018
Rollup of 5 pull requests

Successful merges:

 - #53941 (rustdoc: Sort implementors)
 - #54181 (Suggest && and || instead of 'and' and 'or')
 - #54209 (Partially revert 674a5db "Fix undesirable fallout [from macro modularization]")
 - #54213 (De-overlap the lifetimes of `flow_inits` and `flow_{un,ever_}inits`.)
 - #54244 (Add a small search box to seach Rust's standary library)

Failed merges:

r? @ghost
@bors bors merged commit 2fe4503 into rust-lang:master Sep 16, 2018
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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants