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

Print more tags in rustdoc #37134

Merged
merged 4 commits into from
Nov 9, 2016
Merged

Conversation

GuillaumeGomez
Copy link
Member

@GuillaumeGomez GuillaumeGomez commented Oct 12, 2016

r? @steveklabnik

cc @frewsxcv

A little screenshot:

screen shot 2016-10-13 at 01 41 53

vec!("repr").iter().any(|x| x == &s)
}

fn render_attribute(attr: &clean::Attribute, recurse: bool) -> String {
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Returning Option<String> seems appropriate instead of possibly returning an empty string

}

fn attribute_with_values(s: &str) -> bool {
vec!("repr").iter().any(|x| x == &s)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

["repr"].iter().any(|x| x == &s)

Same goes above

@frewsxcv
Copy link
Member

+0 for me on this. I don't like displaying attributes in the docs in general like we do currently with #[must_use], but at least this makes it a little more consistent than before. Let's see what others think.

@GuillaumeGomez
Copy link
Member Author

cc #37012

@GuillaumeGomez
Copy link
Member Author

I could add a show_attributes button or something like that?

@steveklabnik
Copy link
Member

@bors: delegate=kmcallister

@bors
Copy link
Contributor

bors commented Oct 14, 2016

✌️ @kmcallister can now approve this pull request

@steveklabnik
Copy link
Member

I feel similarly to @frewsxcv .

/cc @rust-lang/tools @rust-lang/docs

@kmcallister
Copy link
Contributor

The screenshot shows unbalanced brackets:

#[repr(i32])]
pub struct F;

@nrc
Copy link
Member

nrc commented Oct 17, 2016

I'd prefer not to do this by default - attributes are mostly an implementation detail and this adds visual clutter.

+1 for a show_attributes option some how.

On the implementation side - why have this ad hoc pretty printing? Why not use the source text? Or the compiler's pretty printer?

@GuillaumeGomez
Copy link
Member Author

@nrc: I actually didn't know there was. And if there was, why not using it from the start? If it's in order to filter the output, we'll still need it.

@GuillaumeGomez
Copy link
Member Author

Ok, so now attributes are hidden by default:

screen shot 2016-11-06 at 22 57 09

And when they're displayed they look like this:

screen shot 2016-11-06 at 22 57 14

cc @rust-lang/docs

@steveklabnik
Copy link
Member

Similarly to #37250, gonna wait until after the beta branch to merge this

@steveklabnik
Copy link
Member

Beta has been branched! Let's see how the wider community feels about this.

@bors: r+

@bors
Copy link
Contributor

bors commented Nov 8, 2016

📌 Commit 520d5f4 has been approved by steveklabnik

eddyb added a commit to eddyb/rust that referenced this pull request Nov 9, 2016
…klabnik

Print more tags in rustdoc

r? @steveklabnik

cc @frewsxcv

A little screenshot:

<img width="1440" alt="screen shot 2016-10-13 at 01 41 53" src="https://cloud.githubusercontent.com/assets/3050060/19331745/873cd71e-90e6-11e6-88f8-715668366a3f.png">
bors added a commit that referenced this pull request Nov 9, 2016
Rollup of 15 pull requests

- Successful merges: #36868, #37134, #37229, #37250, #37370, #37428, #37432, #37472, #37524, #37614, #37622, #37627, #37636, #37644, #37654
- Failed merges: #37463, #37542, #37645
@bors bors merged commit 520d5f4 into rust-lang:master Nov 9, 2016
@GuillaumeGomez GuillaumeGomez deleted the display_tag branch November 10, 2016 10:30
@ollie27 ollie27 mentioned this pull request Mar 7, 2017
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.

6 participants