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

Scrape code examples from examples/ directory for Rustdoc #85833

Merged
merged 24 commits into from
Oct 23, 2021
Merged

Scrape code examples from examples/ directory for Rustdoc #85833

merged 24 commits into from
Oct 23, 2021

Conversation

willcrichton
Copy link
Contributor

@willcrichton willcrichton commented May 30, 2021 •
edited
Loading

Adds support for the functionality described in rust-lang/rfcs#3123

Matching changes to Cargo are here: rust-lang/cargo#9525

Live demo here: https://willcrichton.net/example-analyzer/warp/trait.Filter.html#method.and

@rust-highfive
Copy link
Collaborator

Some changes occurred in HTML/CSS/JS.

cc @GuillaumeGomez

@rust-highfive
Copy link
Collaborator

r? @GuillaumeGomez

(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 May 30, 2021
This was referenced May 30, 2021
Merged
Merged
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@rust-log-analyzer

This comment has been minimized.

Sign in to view
jyn514
jyn514 reviewed Jun 2, 2021
View reviewed changes
src/librustdoc/config.rs Outdated Show resolved Hide resolved
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@bors

This comment has been minimized.

Sign in to view
@bors
Copy link
Contributor

bors commented Jul 8, 2021

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

@bors
Copy link
Contributor

bors commented Jul 19, 2021

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

@inquisitivecrystal inquisitivecrystal added the T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. label Aug 24, 2021
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@BubbaSheen
This comment has been minimized.
@rust-log-analyzer

This comment has been minimized.

Sign in to view
@camelid camelid mentioned this pull request Sep 9, 2021
Open
4 tasks
@willcrichton willcrichton marked this pull request as ready for review September 13, 2021 23:54
@camelid
Copy link
Member

camelid commented Sep 13, 2021

Please rebase over upstream changes instead of merging since rust-lang/rust has a "No-Merge Policy". Thanks!

@willcrichton
Copy link
Contributor Author

@jyn514 I've done a significant cleanup of the PR and addressed some issues raised in the RFC. Notably:

  • Links now go to rustdoc-generated files rather than an external repository.
  • A lot of the highlighting logic has been pushed from the JS run-time to Rustdoc generation-time. I extended the highlighter to take custom decorations as input, which can insert the highlight into the HTML directly rather than after the page has rendered. The JS changes are now substantially smaller as a result.
  • The number of examples for a given function has been capped, and further examples are just links to files (adding very little page weight).
  • Rather than embedding the entire source of an example, I only embed the item enclosing the scraped call site. This further reduces page size.

I also tweaked the UI to make it more visually consistent and easier to use. I'll try to maintain a live demo here as I make changes: https://willcrichton.net/example-analyzer/warp/trait.Filter.html#method.and

I think it's ready for review, unless you (or others) have any big feature requests outstanding.

@camelid
Copy link
Member

camelid commented Sep 14, 2021

Two minor stylistic questions:

  1. [+] More examples is currently in a serif font, whereas the rest of rustdoc usually uses sans-serif for these sorts of things.

  2. Is it intentional that later uses in an example are highlighted in a slightly different color? For example:

    image

@camelid
Copy link
Member

camelid commented Sep 14, 2021 •
edited
Loading

A couple more UI comments:

  1. I'd rather not have the blurred border at the edges of the examples; IMO it doesn't fit in with the rustdoc UI style.

  2. The < and > arrows that are IIRC supposed to be in the top-right of the examples are not visible. I think this is because the DOM bounding boxes of the examples under [+] More examples extends too far to the right.

    EDIT: I think we decided in the RFC discussion that we should remove those errors in favor of just having a source link (at least for now). So I think they can be removed from the generated HTML.

@GuillaumeGomez
Copy link
Member

Looks good to me!

@jyn514 @camelid Want to take a last look before approval?

jyn514
jyn514 approved these changes Oct 20, 2021
View reviewed changes
Copy link
Member

@jyn514 jyn514 left a comment

Choose a reason for hiding this comment

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

This is great, I'm really excited for it to land :) thank you @willcrichton for sticking with this so long! ❤️

camelid
camelid reviewed Oct 20, 2021
View reviewed changes
src/doc/rustdoc/src/unstable-features.md Outdated Show resolved Hide resolved
src/librustdoc/clean/types.rs Outdated Show resolved Hide resolved
@camelid
Copy link
Member

camelid commented Oct 20, 2021

I gave the code a quick look over to see if anything caught my eye. The main thing I'd like changed if possible is #85833 (comment). Other than that, I didn't notice anything major.

Like Joshua said, thank you for sticking with this!

@camelid
Copy link
Member

camelid commented Oct 22, 2021

I haven't looked at the code in detail, but Guillaume and Joshua have, so I don't have any more comments :)

@jyn514
Copy link
Member

jyn514 commented Oct 23, 2021

@bors r+ rollup=iffy

@bors
Copy link
Contributor

bors commented Oct 23, 2021

📌 Commit fd5d614 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 Oct 23, 2021
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Oct 23, 2021
@matthiaskrgr
Rollup merge of rust-lang#85833 - willcrichton:example-analyzer, r=jy…
a7f2bc1 
…n514

Scrape code examples from examples/ directory for Rustdoc

Adds support for the functionality described in rust-lang/rfcs#3123

Matching changes to Cargo are here: rust-lang/cargo#9525

Live demo here: https://willcrichton.net/example-analyzer/warp/trait.Filter.html#method.and
@matthiaskrgr matthiaskrgr mentioned this pull request Oct 23, 2021
Closed
@matthiaskrgr matthiaskrgr mentioned this pull request Oct 23, 2021
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Oct 23, 2021
@bors
Auto merge of rust-lang#90203 - matthiaskrgr:rollup-v215wew, r=matthi…
91b9319 
…askrgr

Rollup of 5 pull requests

Successful merges:

 - rust-lang#85833 (Scrape code examples from examples/ directory for Rustdoc)
 - rust-lang#88041 (Make all proc-macro back-compat lints deny-by-default)
 - rust-lang#89829 (Consider types appearing in const expressions to be invariant)
 - rust-lang#90168 (Reset qualifs when a storage of a local ends)
 - rust-lang#90198 (Add caveat about changing parallelism and function call overhead)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit dcf9242 into rust-lang:master Oct 23, 2021
@rustbot rustbot added this to the 1.58.0 milestone Oct 23, 2021
@Mark-Simulacrum
Copy link
Member

It looks like this was probably the cause of the regression in https://perf.rust-lang.org/compare.html?start=aa5740c715001f981515ed46faaddebf67cb9539&end=91b931926fd49fc97d1e39f2b8206abf1d77ce7d&stat=instructions:u, though it's a fairly minor one. May not be worth investigating too deeply, but marking as a perf-regression for the time being.

@Mark-Simulacrum Mark-Simulacrum added the perf-regression Performance regression. label Oct 26, 2021
@jyn514 jyn514 deleted the example-analyzer branch October 26, 2021 16:20
@willcrichton
Copy link
Contributor Author

@Mark-Simulacrum this change does not trigger by default -- the examples have to be designed to pass in specific flags. I don't think this would affect existing benchmarks.

@jyn514
Copy link
Member

jyn514 commented Oct 26, 2021

@willcrichton the benchmarks are using the default settings, so the regression is real. It pointed to create_renderer, maybe something there is expensive?

That said, I agree this is probably not worth looking into, everything I saw was a one-time cost that should only matter for really tiny crates.

@Mark-Simulacrum Mark-Simulacrum mentioned this pull request Oct 27, 2021
Closed
bors added a commit to rust-lang/cargo that referenced this pull request Oct 28, 2021
@bors
Auto merge of #9525 - willcrichton:example-analyzer, r=alexcrichton
0a98b1d 
Scrape code examples from examples/ directory for Rustdoc

Adds support for the functionality described in rust-lang/rfcs#3123

Matching changes to rustdoc are here: rust-lang/rust#85833
@jplatte jplatte mentioned this pull request Nov 10, 2021
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@camelid camelid camelid left review comments

@jyn514 jyn514 jyn514 approved these changes

@GuillaumeGomez GuillaumeGomez GuillaumeGomez approved these changes

Assignees

@GuillaumeGomez GuillaumeGomez

Labels
perf-regression Performance regression. requires-nightly This issue requires a nightly compiler in some way. 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.58.0
Development

Successfully merging this pull request may close these issues.