-
Notifications
You must be signed in to change notification settings - Fork 12.7k
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] Render tuple fields in structs correctly #80320
[Rustdoc] Render tuple fields in structs correctly #80320
Conversation
Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @ollie27 (or someone else) soon. If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes. Please see the contribution instructions for more information. |
I do think it makes sense for it to be a different color, but it seems that coloring it as a primitive might be confusing. |
I agree, but i suck at design, especially colors 😄 |
☔ The latest upstream changes (presumably #81271) made this pull request unmergeable. Please resolve the merge conflicts. |
@@ -0,0 +1,11 @@ | |||
// @has tuple_struct_fields/struct.Tooople.html |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you also add a check like:
// @has tuple_struct_fields/struct.Tooople.html | |
// @has tuple_struct_fields/struct.Tooople.html | |
// @has - 'Tuple Fields' |
to make sure the heading is correct?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think i added that at one point then removed it because it wasn't working (and the errors are less helpful than staring at a wall), but ill try again tomorrow 👍
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
(and the errors are less helpful than staring at a wall)
Yeah, htmldocck errors could use some work.
As you see fit. I don't have a strong opinion there... |
@@ -3079,21 +3079,22 @@ fn item_struct( | |||
_ => None, | |||
}) | |||
.peekable(); | |||
if let doctree::Plain = s.struct_type { | |||
if let doctree::Plain | doctree::Tuple = s.struct_type { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I didn't even know we could do that in rust! 😮
@@ -0,0 +1,11 @@ | |||
// @has tuple_struct_fields/struct.Tooople.html | |||
// @has - //span '0: usize' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Could you write more complete paths please? It's much easier when an unwanted change occur to find out where it's from in such cases.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As i stated in an earlier comment, i tried that, but the test harness kept erroring and i could not figure out why, which is why i removed it
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I can help with that! :) If it's the same as a normal field, the path should look like this:
//section[@id="main"]/span[@id="structfield.0"] '0: usize'
Also, please add tests to ensure that "Tuple fields" header is there and that the fields are in the sidebar too.
@@ -0,0 +1,11 @@ | |||
// @has tuple_struct_fields/struct.Tooople.html | |||
// @has - //span '0: usize' | |||
// @has - 'Wow! i love tuple fields!' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Same.
// @has tuple_struct_fields/struct.Tooople.html | ||
// @has - //span '0: usize' | ||
// @has - 'Wow! i love tuple fields!' | ||
// @!has - 'I should be invisible' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
👍
Does this fix #42615? (For structs and enums.) |
@RDambrosio016 - ping from triage, can you please address the merge conflict and comments from the reviewers? Thanks. |
I'll see if i can get to it today, ive just been very busy with other things and i kind of forgot about this 😅 |
@RDambrosio016 Ping from triage, any updates on this? |
@RDambrosio016 Ping from triage - unfortunately I'm going to have to close this as inactive. Please reopen this when you're ready to continue. |
…oc, r=jyn514 Add support for tuple struct field documentation Fixes rust-lang#42615. This is rust-lang#80320 updated to new codebase and with added tests. Part of rust-lang#83255. cc `@camelid` (since you were involved on the original PR). r? `@jyn514`
…oc, r=jyn514 Add support for tuple struct field documentation Fixes rust-lang#42615. This is rust-lang#80320 updated to new codebase and with added tests. Part of rust-lang#83255. cc ``@camelid`` (since you were involved on the original PR). r? ``@jyn514``
…oc, r=jyn514 Add support for tuple struct field documentation Fixes rust-lang#42615. This is rust-lang#80320 updated to new codebase and with added tests. Part of rust-lang#83255. cc ```@camelid``` (since you were involved on the original PR). r? ```@jyn514```
This PR makes rustdoc render public tuple fields correctly similar to normal fields, previously no rendering would happen and the description of the field was effectively discarded.
Before:
After:
I was thinking about coloring the
0
a different color, maybe the color of primitives so it stands out more, but id like to hear opinions on that first.