rustdoc: add line breaks to where clauses a la rustfmt #37190

QuietMisdreavus · 2016-10-15T15:51:45Z

Much like my last PR for rustdoc (#36679), this adds line breaks to certain statements based on their line length. Here the focus was on where clauses.

Some examples:

Where clause in a trait function (also in the trait header block at the top of the page)
Where clause on a bare function
Where clauses in trait impls on a struct (scroll to the bottom) These are regularly not on their own line, but will be given their own line now if their "prefix text" doesn't give them enough room to sensibly print their constraints. HashMap's trait impls provide some examples of both behaviors.

The libstd links above are the whole docs rendered with this, and the "bare function" link above is in another set that pulls some notable crates together. petgraph was the one that brought this request up, and that collection also includes itertools which provided an easy sample to test with.

r? @GuillaumeGomez

…nough room

frewsxcv · 2016-10-15T18:14:26Z

src/librustdoc/html/format.rs

@@ -50,7 +50,7 @@ pub struct MutableSpace(pub clean::Mutability);
 #[derive(Copy, Clone)]
 pub struct RawMutableSpace(pub clean::Mutability);
 /// Wrapper struct for emitting a where clause from Generics.
-pub struct WhereClause<'a>(pub &'a clean::Generics);
+pub struct WhereClause<'a>(pub &'a clean::Generics, pub String);


I'd be in favor of making this:

pub struct WhereClause<'a> { pub generics: &'a clean::Generics, pub pad: String, };

So we're explicit about what the fields are instead of a collection of unnamed data structures.

No big deal to me either way. Looking at it again now, I kinda want to change that String (and Method's str, above) to integers since all they're conveying is the number of spaces to prepend onto new lines. They're passed differently, too, but handled the same.

frewsxcv · 2016-10-15T18:15:52Z

src/librustdoc/html/format.rs

+                    clause.push_str(", ");
+                } else {
+                    clause.push_str(",<br>");
+                }


Subjective, but these sort of patterns can be written:

clause.push_str(if f.alternate() { ", " } else { ",<br>" })

Personal preference, I guess. It it part of a broader style guideline somewhere? At that point it's a ternary expression, too, and can be collapsed onto one line.

It it part of a broader style guideline somewhere?

I don't think so; what you have is fine.

frewsxcv · 2016-10-15T18:17:28Z

src/librustdoc/html/format.rs

    if f.alternate() {
        write!(f, "impl{:#} ", i.generics)?;
    } else {
        write!(f, "impl{} ", i.generics)?;
    }
+    plain.push_str(&format!("impl{:#} ", i.generics));


Could this just be:

let mut plain = format!("impl{:#} ", i.generics);

...and then getting rid of the String::new() above?

I don't see why not. I guess my intent was to have the declaration separate since I was going to be pushing more things onto it later.

bluss · 2016-10-16T00:00:30Z

Great work! It's an improvement for sure.

Maybe where clauses that are line broken should always be hanging on impls? Here's an image to illustrate http://i.imgur.com/UjP8aK9.png (2) looks good and (1) is not as good.

ollie27 · 2016-10-16T01:52:52Z

src/librustdoc/html/format.rs

            }
            match pred {
                &clean::WherePredicate::BoundPredicate { ref ty, ref bounds } => {
                    let bounds = bounds;
                    if f.alternate() {
-                        write!(f, "{:#}: {:#}", ty, TyParamBounds(bounds))?;
+                        clause.push_str(&format!("{:#}: {:#}", ty, TyParamBounds(bounds)));


It would be better to use write! instead of .push_str(&format!( to avoid an extra string allocation. For example, by importing std::fmt::Write this can be write!(clause, "{:#}: {:#}", ty, TyParamBounds(bounds)).unwrap();.

I thought about that adjustment as I was going through - I even asked in IRC whether it was possible to use write!() on a String when I was doing it. I decided against writing it in right then because there were other places (not just in my additions here or the previous PR) that used this pattern, so my sense of purism wanted to separate that change to its own logical unit, after this one got dealt with.

QuietMisdreavus · 2016-10-16T02:15:55Z

@bluss To be honest, minutes after I posted this PR I thought about whether that was the way to go. Doing that change would be pretty easy - I'll go ahead and work that in.

bluss · 2016-10-16T16:44:22Z

src/librustdoc/html/render.rs

@@ -2899,10 +2945,12 @@ fn render_impl(w: &mut fmt::Formatter, cx: &Context, i: &Impl, link: AssocItemLi

 fn item_typedef(w: &mut fmt::Formatter, cx: &Context, it: &clean::Item,
                t: &clean::Typedef) -> fmt::Result {
+    let indent = format!("type {}{:#} ", it.name.as_ref().unwrap(), t.generics);
+    let indent = repeat(" ").take(indent.len()).collect::<String>();


There's a new method for this actually, " ".repeat(indent.len()) (-> String). It has the advantage of sizing the allocation correctly upfront.

Oh yeah, I remember you talking about that on IRC. This is a pattern used more broadly than my changes, so I'd be more comfortable taking that to a separate PR? I dunno what the general consensus on that would be, though.

bluss · 2016-10-16T16:49:45Z

Here's a way to count the length of a formatting without making the string. https://is.gd/Yks7tx

QuietMisdreavus · 2016-10-17T14:44:06Z

@bluss On that counting code: Looking at it again, it seems to be just a byte count? That might backfire if non-ascii idents are allowed. On the other hand, I've already been falling into the same trap by using len() myself, so it's not a substantial change from what I've already been doing. I'd be willing to work it in.

bluss · 2016-10-17T14:46:32Z

Yes that's true. It's the same count as String::len's, though.

bluss · 2016-10-23T14:57:54Z

The result looks great, and I'd really like this. I think it would be a lot easier to accept if we had a cleaner way to do the plain/vs regular formatting, without the duplicate code for each piece.

alexcrichton · 2016-11-10T21:05:52Z

@rust-lang/docs thoughts about this PR? It seems to be getting a little old, but it's still mergeable! Would nice to get a resolution!

GuillaumeGomez · 2016-11-11T11:25:51Z

@alexcrichton: I totally forgot about this one. Since people are globally in favor for it, let's merge it. It's still possible in the future to remove it anyway.

@QuietMisdreavus: Thanks for your work and sorry for the delay!

@bors: r+ rollup

bors · 2016-11-11T11:25:52Z

📌 Commit 61cc870 has been approved by GuillaumeGomez

@GuillaumeGomez

…ne, r=GuillaumeGomez rustdoc: add line breaks to where clauses a la rustfmt Much like my last PR for rustdoc (rust-lang#36679), this adds line breaks to certain statements based on their line length. Here the focus was on where clauses. Some examples: - [Where clause in a trait function](https://shiva.icesoldier.me/custom-std/std/iter/trait.Iterator.html?search=#method.unzip) (also in the trait header block at the top of the page) - [Where clause on a bare function](https://shiva.icesoldier.me/doc-custom2/petgraph/visit/fn.depth_first_search.html) - [Where clauses in trait impls on a struct](https://shiva.icesoldier.me/custom-std/std/collections/struct.HashMap.html) (scroll to the bottom) These are regularly not on their own line, but will be given their own line now if their "prefix text" doesn't give them enough room to sensibly print their constraints. HashMap's trait impls provide some examples of both behaviors. The libstd links above are the whole docs rendered with this, and the "bare function" link above is in another set that pulls some notable crates together. `petgraph` was the one that brought this request up, and that collection also includes [itertools](https://shiva.icesoldier.me/doc-custom2/itertools/trait.Itertools.html) which provided an easy sample to test with. r? @GuillaumeGomez

@GuillaumeGomez

…ne, r=GuillaumeGomez rustdoc: add line breaks to where clauses a la rustfmt Much like my last PR for rustdoc (rust-lang#36679), this adds line breaks to certain statements based on their line length. Here the focus was on where clauses. Some examples: - [Where clause in a trait function](https://shiva.icesoldier.me/custom-std/std/iter/trait.Iterator.html?search=#method.unzip) (also in the trait header block at the top of the page) - [Where clause on a bare function](https://shiva.icesoldier.me/doc-custom2/petgraph/visit/fn.depth_first_search.html) - [Where clauses in trait impls on a struct](https://shiva.icesoldier.me/custom-std/std/collections/struct.HashMap.html) (scroll to the bottom) These are regularly not on their own line, but will be given their own line now if their "prefix text" doesn't give them enough room to sensibly print their constraints. HashMap's trait impls provide some examples of both behaviors. The libstd links above are the whole docs rendered with this, and the "bare function" link above is in another set that pulls some notable crates together. `petgraph` was the one that brought this request up, and that collection also includes [itertools](https://shiva.icesoldier.me/doc-custom2/itertools/trait.Itertools.html) which provided an easy sample to test with. r? @GuillaumeGomez

Rollup of 30 pull requests - Successful merges: #37190, #37368, #37481, #37503, #37527, #37535, #37551, #37584, #37600, #37613, #37615, #37659, #37662, #37669, #37682, #37688, #37690, #37692, #37693, #37694, #37695, #37696, #37698, #37699, #37705, #37708, #37709, #37716, #37724, #37727 - Failed merges: #37640, #37689, #37717

…, r=steveklabnik rustdoc: properly calculate line length for where clauses Apparently, while I was cleaning up rust-lang#37190, I regressed the formatting for long where clauses, where it wouldn't take the "prefix" length into account when deciding whether to break the line up. This patch fixes that.

QuietMisdreavus added 4 commits October 13, 2016 10:17

rustdoc: add line breaks to where clauses a la rustfmt

4a6921e

fix spurious </span> appearing before the opening tag

c6ab685

rustdoc: apply proper indent when where clauses aren't on their own line

42f28d3

rustdoc: break where clauses onto their own line if they don't have e…

07b27bb

…nough room

rust-highfive assigned GuillaumeGomez Oct 15, 2016

QuietMisdreavus mentioned this pull request Oct 15, 2016

rustdoc: print non-self arguments of bare functions and struct methods on their own line #36679

Merged

frewsxcv reviewed Oct 15, 2016

View reviewed changes

ollie27 reviewed Oct 16, 2016

View reviewed changes

rustdoc: if a where clause gets any lines, it gets its own line

43abad4

bluss reviewed Oct 16, 2016

View reviewed changes

QuietMisdreavus mentioned this pull request Oct 17, 2016

Improve rustdoc formatting of long function names #34926

Closed

rustdoc: make Method/WhereClause wrappers use usize for indents

61cc870

alexcrichton added the A-docs Area: documentation for any part of the project, including the compiler, standard library, and tools label Nov 10, 2016

eddyb mentioned this pull request Nov 11, 2016

Rollup of 29 pull requests #37726

Closed

eddyb mentioned this pull request Nov 12, 2016

Rollup of 30 pull requests #37730

Merged

bors merged commit 61cc870 into rust-lang:master Nov 12, 2016

QuietMisdreavus deleted the rustdoc-where-newline branch November 14, 2016 14:27

QuietMisdreavus mentioned this pull request Dec 20, 2016

rustdoc: properly calculate line length for where clauses #38497

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

rustdoc: add line breaks to where clauses a la rustfmt #37190

rustdoc: add line breaks to where clauses a la rustfmt #37190

QuietMisdreavus commented Oct 15, 2016

frewsxcv Oct 15, 2016

QuietMisdreavus Oct 15, 2016

frewsxcv Oct 15, 2016

QuietMisdreavus Oct 15, 2016

frewsxcv Oct 15, 2016

frewsxcv Oct 15, 2016

QuietMisdreavus Oct 15, 2016

bluss commented Oct 16, 2016

ollie27 Oct 16, 2016

QuietMisdreavus Oct 16, 2016

QuietMisdreavus commented Oct 16, 2016

bluss Oct 16, 2016 •

edited

Loading

QuietMisdreavus Oct 17, 2016

bluss commented Oct 16, 2016

QuietMisdreavus commented Oct 17, 2016

bluss commented Oct 17, 2016

bluss commented Oct 23, 2016

alexcrichton commented Nov 10, 2016

GuillaumeGomez commented Nov 11, 2016

bors commented Nov 11, 2016

rustdoc: add line breaks to where clauses a la rustfmt #37190

rustdoc: add line breaks to where clauses a la rustfmt #37190

Conversation

QuietMisdreavus commented Oct 15, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bluss commented Oct 16, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

QuietMisdreavus commented Oct 16, 2016

bluss Oct 16, 2016 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

bluss commented Oct 16, 2016

QuietMisdreavus commented Oct 17, 2016

bluss commented Oct 17, 2016

bluss commented Oct 23, 2016

alexcrichton commented Nov 10, 2016

GuillaumeGomez commented Nov 11, 2016

bors commented Nov 11, 2016

bluss Oct 16, 2016 •

edited

Loading