Skip to content

/rust

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 - display `since` version for stable items #30686

Merged
merged 1 commit into from Feb 10, 2016

Conversation

Reviewers
No reviews
Assignees

@steveklabnik steveklabnik

Labels
None yet
Projects
None yet
Milestone
No milestone
7 participants
@wesleywiser @sfackler @tshepang @brson @steveklabnik @bors @bluss
@wesleywiser
Copy link
Member

wesleywiser commented Jan 3, 2016

Here's some screenshots after this change:

screen shot 2016-01-03 at 11 38 30 am
screen shot 2016-01-03 at 11 40 39 am
screen shot 2016-01-03 at 11 42 17 am

I tried to click through the std docs and make sure everything that can have stability attributes has it rendered but I'm probably missing some. I'd also appreciate any feedback on the css changes. I had difficulty getting the since labels aligning correctly for enum variants. If anyone has a better idea for that, I'd be glad to implement it.

Fixes #27607

@rust-highfive rust-highfive assigned steveklabnik Jan 3, 2016

@sfackler

This comment has been minimized.

Sign in to view
Copy link
Member

sfackler commented Jan 3, 2016

Yay!
@tshepang

This comment has been minimized.

Sign in to view
Copy link
Contributor

tshepang commented Jan 3, 2016

I would prefer if the text was less prominent: either greyed or with smaller font. It is too distracting at the moment for something that's so meta.
@wesleywiser

This comment has been minimized.

Sign in to view
Copy link
Member Author

wesleywiser commented Jan 3, 2016

@tshepang What do you think of this?
screen shot 2016-01-03 at 5 14 51 pm
@tshepang

This comment has been minimized.

Sign in to view
Copy link
Contributor

tshepang commented Jan 3, 2016

@wesleywiser that's better, thanks
@brson

This comment has been minimized.

Sign in to view
Copy link
Contributor

brson commented Jan 5, 2016

I also have the sense that this is giving a great deal of prominence to a rather unimportant piece of information.

One thing we could do is remove 'since' - this word doesn't convey any information every time it's repeated.

This is also a new way to style stability information, and I already count two - See for example where stability of the element on the page is indicated by a colored box while stability of its methods is indicated by the word '[Unstable]' while fading out the text. It would be easier to understand that this and that are both types of stability information if they were styled similarly.
@wesleywiser

This comment has been minimized.

Sign in to view
Copy link
Member Author

wesleywiser commented Jan 6, 2016

@brson re 'since': I agree. Here's what it looks like without 'since':
screen shot 2016-01-05 at 9 42 36 pm

I think agree that adding yet another way to show stability information is bad. I'm having some trouble imagining what a common style might look like.

One idea I had that might further minimize the visual noise would be to only show the version number when it is different than the containing object's version number. For example, the previous screenshot would look like this because Option is marked 1.0.0:
screen shot 2016-01-05 at 9 48 41 pm
@tshepang

This comment has been minimized.

Sign in to view
Copy link
Contributor

tshepang commented Jan 6, 2016

I like the second option:

...only show the version number when it is different than the containing object's version number.

@wesleywiser wesleywiser force-pushed the wesleywiser:rustdoc_display_since branch from d2a35e5 to 5c5dab2 Jan 6, 2016

@brson

This comment has been minimized.

Sign in to view
Copy link
Contributor

brson commented Jan 12, 2016

@wesleywiser I do like that idea of not displaying them when they are the same. With that I think I'm ok with trying this.
@steveklabnik

This comment has been minimized.

Sign in to view
Copy link
Member

steveklabnik commented Jan 19, 2016

I basically feel the same as @brson on this thread. Let's give it a try. Let me know when this PR implements that, and we'll merge after the release thursday, assuming you can update it before then.
@wesleywiser

This comment has been minimized.

Sign in to view
Copy link
Member Author

wesleywiser commented Jan 19, 2016

@brson @steveklabnik Will do. Sorry, I've been pretty busy lately but hopefully I'll have some time to work on it later this week. I'll post new screenshots when when I update the PR.
@steveklabnik

This comment has been minimized.

Sign in to view
Copy link
Member

steveklabnik commented Jan 19, 2016

No worries at all :)
@steveklabnik

This comment has been minimized.

Sign in to view
Copy link
Member

steveklabnik commented Feb 8, 2016

@wesleywiser ping! Any chance you've had the time to work on this?
src/librustdoc/clean/mod.rs Outdated
@@ -339,6 +339,14 @@ impl Item {
_ => String::new(),
}
}

pub fn stable_since(&self) -> Option<String> {

This comment has been minimized.

Sign in to view
Copy link
@bluss

bluss Feb 9, 2016

Contributor

Can you use -> Option<&str> here instead? It should be enough since the string is only ever passed down. Change s.since.clone() to &s.since[..] too and update the other impacted code.

I think this change looks great otherwise.

@wesleywiser wesleywiser force-pushed the wesleywiser:rustdoc_display_since branch from 5c5dab2 to 75acee2 Feb 10, 2016

@wesleywiser

This comment has been minimized.

Sign in to view
Copy link
Member Author

wesleywiser commented Feb 10, 2016

@steveklabnik Great timing Steve! I think it's mostly working at this point (I looked over the generated docs and didn't see any glaring issues). I've updated this PR with the new code (and also implemented @bluss's suggestion).
@steveklabnik

This comment has been minimized.

Sign in to view
Copy link
Member

steveklabnik commented Feb 10, 2016

@bors: r+

okay, let's give it a try! 🎊
@bors

This comment has been minimized.

Sign in to view
Copy link
Contributor

bors commented Feb 10, 2016

📌 Commit 75acee2 has been approved by steveklabnik

bors added a commit that referenced this pull request Feb 10, 2016

@bors
Auto merge of #30686 - wesleywiser:rustdoc_display_since, r=steveklabnik 
Here's some screenshots after this change:

![screen shot 2016-01-03 at 11 38 30 am](https://cloud.githubusercontent.com/assets/831192/12079661/23da4e38-b20f-11e5-8c84-ba51d7a59c3f.png)
![screen shot 2016-01-03 at 11 40 39 am](https://cloud.githubusercontent.com/assets/831192/12079663/23e00012-b20f-11e5-9f01-408cc8d43687.png)
![screen shot 2016-01-03 at 11 42 17 am](https://cloud.githubusercontent.com/assets/831192/12079662/23dfe6c2-b20f-11e5-9998-53abc643e2ef.png)

I tried to click through the `std` docs and make sure everything that can have stability attributes has it rendered but I'm probably missing some. I'd also appreciate any feedback on the css changes. I had difficulty getting the `since` labels aligning correctly for enum variants. If anyone has a better idea for that, I'd be glad to implement it.

Fixes #27607
b5da60d
@bors

This comment has been minimized.

Sign in to view
Copy link
Contributor

bors commented Feb 10, 2016

⌛️ Testing commit 75acee2 with merge b5da60d...
@bors

@bors bors merged commit 75acee2 into rust-lang:master Feb 10, 2016
2 checks passed

2 checks passed

continuous-integration/travis-ci/pr The Travis CI build passed
Details
homu Test successful
Details
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
You can’t perform that action at this time.