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

rustdoc: Resolve module-level doc references more locally #65857

Merged
merged 1 commit into from
Nov 1, 2019
Merged

rustdoc: Resolve module-level doc references more locally #65857

merged 1 commit into from
Nov 1, 2019

Conversation

kinnison
Copy link
Contributor

Module level docs should resolve intra-doc links as locally as
possible. As such, this commit alters the heuristic for finding
intra-doc links such that we attempt to resolve names mentioned
in inner documentation comments within the (sub-)module rather
that from the context of its parent.

I'm hoping that this fixes #55364 though right now I'm not sure it's the right fix.

r? @GuillaumeGomez

@JohnTitor JohnTitor added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Oct 27, 2019
@kinnison
Copy link
Contributor Author

Given where the code is, is it worth @QuietMisdreavus or @Manishearth reviewing this?

@GuillaumeGomez
Copy link
Member

With this change, I'm afraid it might not find items located in the parents. Could you provide tests alongside please? (we should have added them from the start...)

Given where the code is, is it worth @QuietMisdreavus or @Manishearth reviewing this?

Their opinion would be very valuable indeed!

@kinnison
Copy link
Contributor Author

With this change, I'm afraid it might not find items located in the parents. Could you provide tests alongside please? (we should have added them from the start...)

I 100% agree with the need for tests, but I'm unsure about how the rustdoc tests quite work -- all the @has stuff.

If you can give me an example of what you mean by "not find items located in the parents" in terms of an example bit of Rust, I'll add tests for the stuff in the original issue, and also your test cases. I'm fairly sure that all the existing stuff passes though.

@kinnison
Copy link
Contributor Author

I found htmldocck.py and have written what I hope is a reasonable test so far. Further guidance gratefully received.

@GuillaumeGomez
Copy link
Member

The test looks good but I'd feel more at ease if you used the syntax I provided you to check the href. :)

Also, do you mind checking with bigger modules levels (like 4 or even 5 levels down?). Thanks in advance!

@kinnison
Copy link
Contributor Author

@GuillaumeGomez To ease review I've pushed two fixup commits. The first changes the XPATHs the way you asked for (and I think they're better this way yes, so thank you). The second adds a five level deep submodule set which refers to sibling modules by means of crate:: and super::super... names to try and cover all bases.

Any other tests you'd like me to add? I'm quite liking the htmldocck.py stuff now.

@kinnison
Copy link
Contributor Author

The third fixup is just to quiet tidy which I didn't realise also ran against test code.

Once things look good, I'll obviously rebase these into a nice commit.

Manishearth
Manishearth approved these changes Oct 30, 2019
View reviewed changes
Copy link
Member

@Manishearth Manishearth left a comment

Choose a reason for hiding this comment

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

r=me with a comment explaining what base_node is

GuillaumeGomez
GuillaumeGomez reviewed Oct 30, 2019
View reviewed changes
src/test/rustdoc/issue-55364.rs Outdated

// @has issue_55364/index.html
// These two functions should refer to each other
// @has - '//section[@id="main"]/table//a[@href="fn.foo.html"]' 'foo'
Copy link
Member

Choose a reason for hiding this comment

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

You're currently checking that the module has a function url. Functions have pages on their own as well. For example: https://doc.rust-lang.org/nightly/std/mem/fn.drop.html

Please update your test to check for the functions' doc and not if the module has a link to the function. :)

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I figure given the above two tests, I can just remove these particular checks. Is that okay?

@GuillaumeGomez
Copy link
Member

Thanks a lot!

@bors: r=Manisheart,GuillaumeGomez

@bors
Copy link
Contributor

bors commented Oct 30, 2019

📌 Commit 33b21d9cd23cb01f2f0bdcf38c2280074b2f070a has been approved by Manisheart,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 Oct 30, 2019
@kinnison
rustdoc: Resolve module-level doc references more locally
c24a099 
Module level docs should resolve intra-doc links as locally as
possible.  As such, this commit alters the heuristic for finding
intra-doc links such that we attempt to resolve names mentioned
in *inner* documentation comments within the (sub-)module rather
that from the context of its parent.

Signed-off-by: Daniel Silverstone <dsilvers@digital-scurf.org>
@kinnison
Copy link
Contributor Author

I've pushed a squashed commit so there's no fixup! in the history

@GuillaumeGomez
Copy link
Member

@bors: r=Manisheart,GuillaumeGomez

@bors
Copy link
Contributor

bors commented Oct 31, 2019

📌 Commit c24a099 has been approved by Manisheart,GuillaumeGomez

tmandry added a commit to tmandry/rust that referenced this pull request Oct 31, 2019
@tmandry
Rollup merge of rust-lang#65857 - kinnison:kinnison/issue-55364, r=Ma…
ee58559 
…nisheart,GuillaumeGomez

rustdoc: Resolve module-level doc references more locally

Module level docs should resolve intra-doc links as locally as
possible.  As such, this commit alters the heuristic for finding
intra-doc links such that we attempt to resolve names mentioned
in *inner* documentation comments within the (sub-)module rather
that from the context of its parent.

I'm hoping that this fixes rust-lang#55364 though right now I'm not sure it's the right fix.

r? @GuillaumeGomez
@tmandry tmandry mentioned this pull request Oct 31, 2019
Closed
bors added a commit that referenced this pull request Oct 31, 2019
@bors
Auto merge of #66009 - tmandry:rollup-jt3wtfr, r=tmandry
a143325 
Rollup of 14 pull requests

Successful merges:

 - #65112 (Add lint and tests for unnecessary parens around types)
 - #65459 (Fix `-Zunpretty=mir-cfg` to render multiple items)
 - #65471 (Add long error explanation for E0578)
 - #65857 (rustdoc: Resolve module-level doc references more locally)
 - #65914 (Use structured suggestion for unnecessary bounds in type aliases)
 - #65945 (Optimize long-linker-command-line test)
 - #65946 (Make `promote_consts` emit the errors when required promotion fails)
 - #65960 (doc: reword iter module example and mention other methods)
 - #65963 (update submodules to rust-lang)
 - #65972 (Fix libunwind build: Define __LITTLE_ENDIAN__ for LE targets)
 - #65977 (Fix incorrect diagnostics for expected type in E0271 with an associated type)
 - #65995 (Add error code E0743 for "C-variadic has been used on a non-foreign function")
 - #65997 (Fix outdated rustdoc of Once::init_locking function)
 - #66005 (vxWorks: remove code related unix socket)

Failed merges:

r? @ghost
tmandry added a commit to tmandry/rust that referenced this pull request Nov 1, 2019
@tmandry
Rollup merge of rust-lang#65857 - kinnison:kinnison/issue-55364, r=Ma…
48a9f59 
…nisheart,GuillaumeGomez

rustdoc: Resolve module-level doc references more locally

Module level docs should resolve intra-doc links as locally as
possible.  As such, this commit alters the heuristic for finding
intra-doc links such that we attempt to resolve names mentioned
in *inner* documentation comments within the (sub-)module rather
that from the context of its parent.

I'm hoping that this fixes rust-lang#55364 though right now I'm not sure it's the right fix.

r? @GuillaumeGomez
@tmandry tmandry mentioned this pull request Nov 1, 2019
Merged
bors added a commit that referenced this pull request Nov 1, 2019
@bors
Auto merge of #66021 - tmandry:rollup-y13l6n9, r=tmandry
87cbf0a 
Rollup of 16 pull requests

Successful merges:

 - #65112 (Add lint and tests for unnecessary parens around types)
 - #65470 (Don't hide ICEs from previous incremental compiles)
 - #65471 (Add long error explanation for E0578)
 - #65857 (rustdoc: Resolve module-level doc references more locally)
 - #65902 (Make ItemContext available for better diagnositcs)
 - #65914 (Use structured suggestion for unnecessary bounds in type aliases)
 - #65946 (Make `promote_consts` emit the errors when required promotion fails)
 - #65960 (doc: reword iter module example and mention other methods)
 - #65963 (update submodules to rust-lang)
 - #65972 (Fix libunwind build: Define __LITTLE_ENDIAN__ for LE targets)
 - #65977 (Fix incorrect diagnostics for expected type in E0271 with an associated type)
 - #65995 (Add error code E0743 for "C-variadic has been used on a non-foreign function")
 - #65997 (Fix outdated rustdoc of Once::init_locking function)
 - #66002 (Stabilize float_to_from_bytes feature)
 - #66005 (vxWorks: remove code related unix socket)
 - #66018 (Revert PR 64324: dylibs export generics again (for now))

Failed merges:

r? @ghost
@bors bors merged commit c24a099 into rust-lang:master Nov 1, 2019
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@GuillaumeGomez GuillaumeGomez GuillaumeGomez left review comments

@Manishearth Manishearth Manishearth approved these changes

Assignees

@GuillaumeGomez GuillaumeGomez

Labels
S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

rustdoc intra links: Module-level doc uses parent module's namespace instead of own
5 participants
@kinnison @GuillaumeGomez @bors @Manishearth @JohnTitor