Skip to content

rustdoc: Don't add extra newlines for fully opaque structs #35667

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

Merged
merged 1 commit into from
Sep 14, 2016

Conversation

ollie27
Copy link
Member

@ollie27 ollie27 commented Aug 14, 2016

Changes the definition for braced structs with only private or hidden fields to save space on the page.

Before:

pub struct Vec<T> {
    // some fields omitted
}

After:

pub struct Vec<T> { /* fields omitted */ }

This also cleans up empty braced structs.

Before:

pub struct Foo {
}

After:

pub struct Foo {}

before after

cc #34713

@rust-highfive
Copy link
Contributor

r? @steveklabnik

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

@steveklabnik
Copy link
Member

I like it, but want to talk about it with the docs team

@peschkaj
Copy link

I like this as well.

@strega-nil
Copy link
Contributor

strega-nil commented Aug 17, 2016

I'm against until it adds back the Struct part of Struct std::vec::Vec in the title.

@GuillaumeGomez
Copy link
Member

GuillaumeGomez commented Aug 17, 2016

Same as @ubsan.

@ollie27
Copy link
Member Author

ollie27 commented Aug 17, 2016

@ubsan Sorry, that's an unrelated change: #35003. You can just ignore the title as far as this PR is concerned.

@steveklabnik
Copy link
Member

Concerns raised in the docs meeting:

  1. That this change also removes the struct bit, like in rustdoc: Remove item types from some title pages #35003 . That bit is controversial, so we might not want to merge this with that change. (Though, @ollie27 it seems like you just posted that this actually from the other PR, so not a real problem)
  2. .. is already rust syntax, so this is a bit ambiguous. Struct(..) at least shows as Struct(RangeFull), but users may not know that.
  3. Related, when there are private fields, this means that you can't use struct literal syntax to create the struct. So it's important to keep this distinction in some form.

So, we are vaguely 👍 on this idea, but have some concerns. Let's keep discussing this in-issue.

@ollie27
Copy link
Member Author

ollie27 commented Aug 17, 2016

  1. Yeah maybe I shouldn't have use the same repo to test both of those changes.
  2. I was aiming for it to match rust syntax. Specifically a pattern that would match the struct which is what we do for tuple structs with hidden fields: std::process::Stdio. I could add another . so it becomes a full ellipsis like we do for default trait methods: std::iter::Iterator if that's less confusing.
  3. Right, I forgot about the case of a braced struct with no fields. I'll fix that.

@ollie27
Copy link
Member Author

ollie27 commented Aug 17, 2016

I've fixed the case of empty braced structs. Now they're displayed as pub struct Foo {} rather than:

pub struct Foo {
}

@peschkaj
Copy link

@ollie27 could you re-render the "after" picture so we can easily look at the change?

@ollie27
Copy link
Member Author

ollie27 commented Aug 20, 2016

I've updated the PR message to hopefully clear things up. Sorry for causing so much confusion.

@samlh
Copy link

samlh commented Sep 4, 2016

I like this, but would vaguely prefer

pub struct Vec<T> { /*...*/ }

@steveklabnik
Copy link
Member

steveklabnik commented Sep 7, 2016

@ollie27 we talked about this at the docs team meeting, and we like this, but would prefer this:

pub struct Vec<T> { /* fields omitted */ }

Can you change it to that, then? We can merge after.

Changes the definition for opaque structs to look like `pub struct Vec<T>
{ /* fields omitted */ }` to save space on the page.

Also only use one line for empty braced structs.
@ollie27 ollie27 force-pushed the rustdoc_opaque_structs branch from d7c4171 to 8154a6b Compare September 9, 2016 02:03
@ollie27 ollie27 changed the title rustdoc: Don't add "some fields omitted" for fully opaque structs rustdoc: Don't add extra newlines for fully opaque structs Sep 9, 2016
@ollie27
Copy link
Member Author

ollie27 commented Sep 9, 2016

I've changed it to pub struct Vec<T> { /* fields omitted */ }.

@GuillaumeGomez
Copy link
Member

👍

@steveklabnik
Copy link
Member

@bors: r+

thank you so much!

@bors
Copy link
Collaborator

bors commented Sep 14, 2016

📌 Commit 8154a6b has been approved by steveklabnik

@bors
Copy link
Collaborator

bors commented Sep 14, 2016

⌛ Testing commit 8154a6b with merge 97b561a...

bors added a commit that referenced this pull request Sep 14, 2016
rustdoc: Don't add extra newlines for fully opaque structs

Changes the definition for braced structs with only private or hidden fields to save space on the page.

Before:
```
pub struct Vec<T> {
    // some fields omitted
}
```
After:
```
pub struct Vec<T> { /* fields omitted */ }
```

This also cleans up empty braced structs.

Before:
```
pub struct Foo {
}
```
After:
```
pub struct Foo {}
```

[before](https://doc.rust-lang.org/nightly/std/vec/struct.Vec.html) [after](https://ollie27.github.io/rust_doc_test/std/vec/struct.Vec.html)

cc #34713
@bors bors merged commit 8154a6b into rust-lang:master Sep 14, 2016
@ollie27 ollie27 deleted the rustdoc_opaque_structs branch September 18, 2016 11:47
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

8 participants