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: Unify where clause styling #113043

Closed
wants to merge 10 commits into from

Conversation

camelid
Copy link
Member

@camelid camelid commented Jun 26, 2023

Fixes #112901 (and other inconsistencies).

After this change, all where clauses are rendered the same (except for ending
in commas versus semicolons in some cases). They look like this (note that the
where clause will also be in a slightly-smaller font size than is used for the
rest of the docs):

pub trait Tr {
    type F<T>
    where
        T: Clone;
}

r? @GuillaumeGomez

I cannot find a reason for this `truncate` call, and all tests pass with
it removed. I assume it was added to reduce the allocation size
(capacity) of the `String`, but the docs explicitly say it has no effect
on capacity.
This commit (or one of the prior ones) mostly fixes rust-lang#112901, though it
causes one case (item-decl for trait assoc ty) to regress in that issue.
@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. labels Jun 26, 2023
@camelid
Copy link
Member Author

camelid commented Jun 26, 2023

EDIT: see #113043 (comment) for full-page before/after comparison

Screenshot (will post more later):

image

It's a lot of snapshots, but it's quite hard to test this sort of
formatting with other ways.
@camelid
Copy link
Member Author

camelid commented Jun 26, 2023

"Long" before (1.65.0)

image

Before (nightly 2023-06-24)

image

After (this PR)

image

@camelid
Copy link
Member Author

camelid commented Jun 26, 2023

Before (nightly 2023-06-24)

image

After (this PR)

image

@camelid
Copy link
Member Author

camelid commented Jun 26, 2023

Before (nightly 2023-06-24)

image

After (this PR)

image

@GuillaumeGomez
Copy link
Member

I think your change breaks alignment starting from the second bound if you have more than one bound since you changed the font size. For example:

pub struct Foo<A, B>(A, B)
where
    A: Display,
    B: Clone;

We should add a GUI test for that.

For the broken indent (more than 4 whitespace characters diff between where and bounds), there was #112927.

As for this indent:

pub trait Foo {
    fn bar<T>(t: T)
       where T: Clone;
}

I thought it was voluntary so I left it as is. But you can fix it by removing let where_indent = 3; (and its uses). Investigating.

@fmease
Copy link
Member

fmease commented Jun 26, 2023

The three space indentation was introduced in #102842. See #85566 for the rationale. I can see where it's coming from but I dislike that it meant no longer formatting where-clauses in adherence with the Rust style guide.

@notriddle
Copy link
Contributor

I get where you're coming from with wanting to follow the style guide, but I do not want to endlessly re-litigate this. Whichever of these two options you don't like, I'm sure you agree that thrashing between the two every few months is even worse.

I'm not going to approve a PR that reverts #102842 (comment) unless something has happened since then to make bringing it up again worthwhile. There are plenty of things that could invalidate the decision: a field report from some educator showing that the current behavior is confusing to new users, a redesign that makes it less important to format where clauses in a compact way, or a new proposal that wasn't evaluated at the time, just as a few examples.

@camelid
Copy link
Member Author

camelid commented Jun 26, 2023

I'm fine with switching the formatting back to what it was before. I went with this style guide–based formatting initially because it was unclear to me what the intended style was, since there were inconsistencies.

@camelid
Copy link
Member Author

camelid commented Jun 26, 2023

For the broken indent (more than 4 whitespace characters diff between where and bounds), there was #112927.

The specific case that this PR fixes was not handled by that PR.

@camelid
Copy link
Member Author

camelid commented Jun 26, 2023

So just to make sure I understand the formatting:

  • trait methods and other associated items are formatted as in rustdoc: change trait bound formatting #102842 -- but in small font or normal font?
  • everything else is formatted the rustfmt way -- in small font (this is a small change since previously, some used small font and some didn't, for no apparent reason)

EDIT: specifically, trait methods and assoc items that are in the item-decl get the special formatting. Outside of the item-decl, it seems they're currently formatted the same way as everything else

@GuillaumeGomez
Copy link
Member

The specific case that this PR fixes was not handled by that PR.

You mean removing the 3 whitespace characters indent?

@camelid
Copy link
Member Author

camelid commented Jun 26, 2023

No, #112901 is that some items have a 9-space indent for their where-clause.

@notriddle
Copy link
Contributor

notriddle commented Jun 26, 2023

trait methods and other associated items are formatted as in #102842 -- but in small font or normal font?

It has to be normal font, because the three space indentation is designed to line these up:

fn whatever<T>(x: T)
   where T: Read
---^ align here

everything else is formatted the rustfmt way -- in small font (this is a small change since previously, some used small font and some didn't, for no apparent reason)

That's probably a good idea.

@fmease
Copy link
Member

fmease commented Jun 26, 2023

but I do not want to endlessly re-litigate this

Aye, makes sense. I didn't notice that this decision was FCP'ed. All good then, I defo don't want to needlessly reopen the case.

I'm happy as long as where-clauses outside of .item-decl have a consistent style (small font, where not indented, bounds w/ an indent of 4). AFAIU, the FCP was only about the where-clauses inside .item-decls.

@notriddle
Copy link
Contributor

I'm happy as long as where-clauses outside of .item-decl have a consistent style (small font, where not indented, bounds w/ an indent of 4). AFAIU, the FCP was only about the where-clauses inside .item-decls.

Agree. It makes no sense to try to hyper-optimize the docblocks for compactness. That only makes sense for item-decl. They should probably be different functions.

@Dylan-DPC
Copy link
Member

@camelid any updates on this? thanks

@camelid
Copy link
Member Author

camelid commented Aug 19, 2023

Sorry, I haven't had a chance to work on this since June. I think the main thing is for us (T-rustdoc) to agree on what we want our where clause styling to be across rustdoc. My proposal is as follows:

  • On a trait's page, in its item-decl (the pseudocode block at the top of the page), the where clauses for its associated items have normal font-size and 3-space indent. The first bound is on the same line as the where keyword, and subsequent lines are indented so the bounds line up. This is to maintain consistency with what was agreed upon in rustdoc: change trait bound formatting #102842. In addition, where clauses on the trait itself (i.e., trait Foo where ...) will be styled the same way.

  • Everywhere else that where clauses appear (in the rest of traits' pages, in other items' item-decls, and in the rest of the pages for other items), they are in a slightly smaller font, with rustfmt style: The where keyword has no indent with respect to its attached item and is on its own line, and each subsequent line (with one bound each) has 4-space indent.

@GuillaumeGomez @notriddle (and anyone else who's interested) does this sound good to you?

@GuillaumeGomez
Copy link
Member

Sounds good to me.

@camelid camelid added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Aug 23, 2023
@bors
Copy link
Contributor

bors commented Oct 4, 2023

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

@Dylan-DPC
Copy link
Member

@camelid any updates on this?

@Dylan-DPC
Copy link
Member

Closing this as inactive. Feel free to reöpen this pr or create a new pr if you get the time to work on this. Thanks

@Dylan-DPC Dylan-DPC closed this Mar 27, 2024
@Dylan-DPC Dylan-DPC added S-inactive Status: Inactive and waiting on the author. This is often applied to closed PRs. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Mar 27, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-inactive Status: Inactive and waiting on the author. This is often applied to closed PRs. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

rustdoc: where-clauses on tuple & unit structs and on assoc tys are weirdly styled
7 participants