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.

Sign up for GitHub

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

Jump to bottom

Remove usage of global variable "inlined_types" #68192

Merged
merged 2 commits into from
Jan 22, 2020
Merged

Remove usage of global variable "inlined_types" #68192

merged 2 commits into from
Jan 22, 2020

Conversation

GuillaumeGomez
Copy link
Member

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

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

@GuillaumeGomez
Copy link
Member Author

So let's go for @kinnison then!

r? @kinnison

cc @ollie27 as well. :)

@GuillaumeGomez
Copy link
Member Author

Fixed the tidy issue.

kinnison
kinnison reviewed Jan 14, 2020
View reviewed changes
Copy link
Contributor

@kinnison kinnison left a comment

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?

src/librustdoc/html/render.rs Show resolved Hide resolved
src/librustdoc/html/render.rs Show resolved Hide resolved
@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
Copy link
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
Copy link
Member Author

Added the test.

kinnison
kinnison reviewed Jan 20, 2020
View reviewed changes
Copy link
Contributor

@kinnison kinnison left a comment

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
Copy link
Member Author

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

@bors: r=kinnison

@bors
Copy link
Contributor

bors commented Jan 22, 2020

📌 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
Copy link
Contributor

bors commented Jan 22, 2020

⌛ 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
Copy link
Contributor

bors commented Jan 22, 2020

☀️ 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

@kinnison kinnison kinnison left review comments

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 @pietroalbini @bors @kinnison @JohnTitor