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

Improve associated constant rendering in rustdoc #39944

Merged
merged 4 commits into from
Mar 1, 2017

Conversation

GuillaumeGomez
Copy link
Member

Before:

screen shot 2017-02-19 at 00 30 51

After:

screen shot 2017-02-19 at 00 30 39

cc @SergioBenitez

r? @rust-lang/docs

@frewsxcv
Copy link
Member

LGTM. Gonna hold off on doing an r+ to see if others have feedback.

@SergioBenitez
Copy link
Contributor

Definitely nicer! My only remaining concern is that long definitions still look quite bad. As an example, see ContentType::JSON. If the definition remains on the same line as the name of the constant, the rendering won't look great.

To ameliorate this, I propose that we adopt one of the following:

  1. Hide the definition entirely on the main page. Clicking on the constant's name leads to a page that shows the full definition as well as the documentation.
  2. Hide the definition by default but allow users to click an "expand" button to see the definition.
  3. Perhaps in conjunction with 2, move the definition to the body of the documentation, where the docstring is shown. Place the definition inside of a shaded pre section.

@GuillaumeGomez
Copy link
Member Author

I feel a bit off for hiding type and value information by default. In your case, the indent will allow to make the reading easier (which is helped by the new style).

@steveklabnik
Copy link
Member

Seems good to me too.

@QuietMisdreavus
Copy link
Member

Not quite a fan, but probably better than nothing. I like @SergioBenitez's suggestion 3, though I'm not sure how rustdoc receives the values of associated consts. It might take some finesse (read: duplicate code) to get it to render through the same pretty-print as a struct's definition.

@GuillaumeGomez
Copy link
Member Author

I just thought that moving associated consts in a "category" of their own might be problematic in case this value is only for certain implementation (I think about type parameters specialization).

@SergioBenitez
Copy link
Contributor

That's not what I'm proposing, @GuillaumeGomez. Keep the const documentation where it is, relatively, but make it look like:

const Other: usize

const Other: usize = 12

This is the documentation for this item.

A larger example:

const Any: ContentType

const Any: ContentType = ContentType{ttype: UncasedAscii{string: Cow::Borrowed("*"),},
           subtype: UncasedAscii{string: Cow::Borrowed("*"),},
           params: None,}

ContentType representing any Content-Type (*/*).

You can play with the formatting and optionally hide the definition by default.

@GuillaumeGomez
Copy link
Member Author

Ok, sorry the misunderstanding! This way seems to be quite good indeed. I'll give it a try.

@GuillaumeGomez
Copy link
Member Author

Ok, I updated as suggested:

Hidden:

screen shot 2017-02-26 at 18 33 12

Not hidden:

screen shot 2017-02-26 at 18 33 14

By default they're visible.

@SergioBenitez
Copy link
Contributor

@GuillaumeGomez I think we still want to know the type in the non-hidden version, right?

@frewsxcv
Copy link
Member

Looking great @GuillaumeGomez, nice job :)

@SergioBenitez
Copy link
Contributor

Actually, if the definition is visible by default, maybe it's okay to not include the type in the hidden version?

@GuillaumeGomez
Copy link
Member Author

I'll need to add tests as well.

@GuillaumeGomez
Copy link
Member Author

So, I added all missing types in the "correct" output and added tests (and filed an ICE, so nice of me).

@steveklabnik
Copy link
Member

@SergioBenitez what do you think?

@frewsxcv
Copy link
Member

@bors r+

@bors
Copy link
Contributor

bors commented Feb 28, 2017

📌 Commit bd704ba has been approved by frewsxcv

@bors
Copy link
Contributor

bors commented Feb 28, 2017

⌛ Testing commit bd704ba with merge 9b19645...

@frewsxcv
Copy link
Member

@bors retry

@bors
Copy link
Contributor

bors commented Feb 28, 2017

⌛ Testing commit bd704ba with merge f7cc0c6...

@alexcrichton
Copy link
Member

@bors: retry

Looks like travis missed this PR...

frewsxcv added a commit to frewsxcv/rust that referenced this pull request Mar 1, 2017
…=frewsxcv

Improve associated constant rendering in rustdoc

Before:

<img width="1440" alt="screen shot 2017-02-19 at 00 30 51" src="https://cloud.githubusercontent.com/assets/3050060/23097697/caeed80e-f63a-11e6-98c2-5d27e4efd76d.png">

After:

<img width="1440" alt="screen shot 2017-02-19 at 00 30 39" src="https://cloud.githubusercontent.com/assets/3050060/23097698/cfb4874e-f63a-11e6-80cf-ffbf5c5c6162.png">

cc @SergioBenitez

r? @rust-lang/docs
bors added a commit that referenced this pull request Mar 1, 2017
Rollup of 6 pull requests

- Successful merges: #39419, #39936, #39944, #39960, #40028, #40128
- Failed merges:
bors added a commit that referenced this pull request Mar 1, 2017
Rollup of 6 pull requests

- Successful merges: #39419, #39936, #39944, #39960, #40028, #40128
- Failed merges:
@bors bors merged commit bd704ba into rust-lang:master Mar 1, 2017
@GuillaumeGomez GuillaumeGomez deleted the associated-consts branch March 1, 2017 10:13
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.

7 participants