Skip to content

Remove usage of global variable "inlined_types" - #68192

Merged
bors merged 2 commits into
rust-lang:masterfrom
GuillaumeGomez:remove-inlined-types
Jan 22, 2020
Merged

Remove usage of global variable "inlined_types"#68192
bors merged 2 commits into
rust-lang:masterfrom
GuillaumeGomez:remove-inlined-types

Conversation

@GuillaumeGomez

@GuillaumeGomez GuillaumeGomez commented Jan 13, 2020

Copy link
Copy Markdown
Member

@JohnTitor JohnTitor added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jan 13, 2020
@emilyalbini

emilyalbini commented Jan 14, 2020

Copy link
Copy Markdown
Member

Thanks for the PR! I don't feel I'm a good reviewer for it though (never touched rustdoc myself).

@GuillaumeGomez

GuillaumeGomez commented Jan 14, 2020

Copy link
Copy Markdown
Member Author

So let's go for @kinnison then!

r? @kinnison

cc @ollie27 as well. :)

@GuillaumeGomez

GuillaumeGomez commented Jan 14, 2020

Copy link
Copy Markdown
Member Author

Fixed the tidy issue.

kinnison
kinnison reviewed Jan 14, 2020
View reviewed changes

@kinnison kinnison left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I see the principal of what is going on here, and other than the two less nice to read strings I'm OK with the implementation. I'm going to assume we have some tests covering the presence of synthetic impls, but is it worth writing a test which ensures the aliases=... attribute is appearing properly?

Comment thread src/librustdoc/html/render.rs Outdated
Comment thread src/librustdoc/html/render.rs Outdated
@kinnison kinnison added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Jan 14, 2020
@GuillaumeGomez

GuillaumeGomez commented Jan 15, 2020

Copy link
Copy Markdown
Member Author

About the aliases check, it has do be done inside a browser to check that the UI doesn't change. However, I'll add a test to check that the attribute is correctly generated.

@GuillaumeGomez

GuillaumeGomez commented Jan 20, 2020

Copy link
Copy Markdown
Member Author

Added the test.

kinnison
kinnison reviewed Jan 20, 2020
View reviewed changes

@kinnison kinnison left a comment

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

As far as I can tell, the UI is not broken by this -- I've reviewed a number of crates' docs which I'm at least passingly familiar with and things look sane.

@bors r+

@GuillaumeGomez

GuillaumeGomez commented Jan 22, 2020

Copy link
Copy Markdown
Member Author

@kinnison You need to comment apparently, bors doesn't detect on review comments I think.

@bors: r=kinnison

@bors

bors commented Jan 22, 2020

Copy link
Copy Markdown
Collaborator

📌 Commit 871e82b has been approved by kinnison

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Jan 22, 2020
@bors

bors commented Jan 22, 2020

Copy link
Copy Markdown
Collaborator

⌛ Testing commit 871e82b with merge d1e594f...

bors added a commit that referenced this pull request Jan 22, 2020
@bors
Auto merge of #68192 - GuillaumeGomez:remove-inlined-types, r=kinnison
d1e594f 
Remove usage of global variable "inlined_types"

r? @pietroalbini
@bors

bors commented Jan 22, 2020

Copy link
Copy Markdown
Collaborator

☀️ Test successful - checks-azure
Approved by: kinnison
Pushing d1e594f to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Jan 22, 2020
@bors
bors merged commit 871e82b into rust-lang:master Jan 22, 2020
@GuillaumeGomez
GuillaumeGomez deleted the remove-inlined-types branch January 23, 2020 12:33
@ollie27 ollie27 mentioned this pull request Jan 27, 2020
Closed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

1 more reviewer
@kinnison kinnison kinnison left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

@kinnison kinnison

Labels

merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

5 participants

@GuillaumeGomez @emilyalbini @bors @kinnison @JohnTitor