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 is_spotlight field from Trait #80914

Merged
merged 5 commits into from
Feb 24, 2021
Merged

Remove is_spotlight field from Trait #80914

merged 5 commits into from
Feb 24, 2021

Conversation

GuillaumeGomez
Copy link
Member

@GuillaumeGomez GuillaumeGomez commented Jan 11, 2021 •
edited
Loading

Small PR, only the last commit is relevant here. The rest is coming from #80883 because I need the TyCtxt stored inside Cache.

The point is to make ItemKind looks as close as possible to the compiler type so that it makes the switch simpler (which is why I make all these "small" PRs).

r? @jyn514

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jan 11, 2021
@GuillaumeGomez GuillaumeGomez force-pushed the remove-is_spotlight branch 2 times, most recently from 7254d97 to 2ef594d Compare January 11, 2021 21:34
@jyn514
Copy link
Member

jyn514 commented Jan 12, 2021

@camelid would you like to review?

@camelid camelid added the T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. label Jan 12, 2021
@camelid
Copy link
Member

camelid commented Jan 12, 2021

@jyn514 Thanks for giving me the opportunity to review! :)

I'll give it a look, but it's probably still a good idea for you to take a look as well since I'm still learning the intricacies of rustdoc.

@camelid camelid added the S-blocked Status: Marked as blocked ❌ on something else such as an RFC or other implementation work. label Jan 12, 2021
@camelid
Copy link
Member

camelid commented Jan 12, 2021

(Marking as blocked on #80883.)

camelid
camelid reviewed Jan 12, 2021
View reviewed changes
Copy link
Member

@camelid camelid left a comment

Choose a reason for hiding this comment

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

Is this field being removed because it's only used in one place and we don't want to pile stuff onto clean::Trait?

src/librustdoc/clean/inline.rs Show resolved Hide resolved
src/librustdoc/clean/utils.rs Outdated Show resolved Hide resolved
src/librustdoc/html/render/mod.rs Outdated Show resolved Hide resolved
This was referenced Jan 12, 2021
Closed
Merged
@GuillaumeGomez GuillaumeGomez mentioned this pull request Jan 13, 2021
Merged
@bors
Copy link
Contributor

bors commented Jan 21, 2021

☔ The latest upstream changes (presumably #81240) made this pull request unmergeable. Please resolve the merge conflicts.

bors added a commit to rust-lang-ci/rust that referenced this pull request Jan 27, 2021
@bors
Auto merge of rust-lang#80987 - GuillaumeGomez:remove-cache-key, r=jy…
613ef74 
…n514

Remove CACHE_KEY global

We realized in rust-lang#80914 that the cache handling (through a global) needed to be updated to make it much easier to handle.

r? `@jyn514`
@GuillaumeGomez
Copy link
Member Author

Rebased! Only the last commit is from this PR, the others still come from #80883.

cc @CraftSpider @ollie27
r? @camelid

@rust-highfive rust-highfive assigned camelid and unassigned jyn514 Jan 28, 2021
@rust-log-analyzer

This comment has been minimized.

Sign in to view
CraftSpider
CraftSpider reviewed Jan 28, 2021
View reviewed changes
Copy link
Contributor

@CraftSpider CraftSpider left a comment

Choose a reason for hiding this comment

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

Looks good. As far as I can tell this should behave the same, just with spotlight calculated at use instead of early. Only question is: How is spotlight_decl called, will this end up duplicating the spotlight lookup? Not too terrible if it does, but want to make sure I understand.

@GuillaumeGomez GuillaumeGomez changed the title Remove is spotlight field from Trait Remove is_spotlight field from Trait Jan 28, 2021
@GuillaumeGomez
Copy link
Member Author

That's actually a pretty good point. It forces us to run this check every time a function returns a "spotlighted" item. That's not great.

@GuillaumeGomez
* Fix some typo
3c59e64 
* Improve documentation
* Add a test to ensure that spotlighted traits from dependencies are taken into account as expected
@GuillaumeGomez
Improve code readability
33aaead
@GuillaumeGomez
Copy link
Member Author

r=me with nits fixed and the description updated - I don't think this needs to mention #80883 now that it's been merged.

I updated the description but I'll keep the mention to the other PR. ;)

@GuillaumeGomez
Copy link
Member Author

@bors: r=jyn514

@bors
Copy link
Contributor

bors commented Feb 23, 2021

📌 Commit 33aaead has been approved by jyn514

@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 Feb 23, 2021
@bors
Copy link
Contributor

bors commented Feb 24, 2021

⌛ Testing commit 33aaead with merge f46c0e49c6121d2ea31376c25c1ac1b7b91d0c91...

@bors
Copy link
Contributor

bors commented Feb 24, 2021

💥 Test timed out

@bors bors added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Feb 24, 2021
@GuillaumeGomez
Copy link
Member Author

@bors: retry

@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-review Status: Awaiting review from the assignee but also interested parties. labels Feb 24, 2021
@bors
Copy link
Contributor

bors commented Feb 24, 2021

⌛ Testing commit 33aaead with merge a8486b6...

@bors
Copy link
Contributor

bors commented Feb 24, 2021

☀️ Test successful - checks-actions
Approved by: jyn514
Pushing a8486b6 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Feb 24, 2021
@bors bors merged commit a8486b6 into rust-lang:master Feb 24, 2021
@rustbot rustbot added this to the 1.52.0 milestone Feb 24, 2021
This was referenced Feb 24, 2021
Closed
Merged
Merged
@GuillaumeGomez GuillaumeGomez deleted the remove-is_spotlight branch February 24, 2021 18:15
@camelid camelid mentioned this pull request Apr 1, 2021
Merged
camelid added a commit to camelid/rust that referenced this pull request Apr 2, 2021
@camelid
rustdoc: Rename internal uses of spotlight
5f96c48 
I didn't make these renames in rust-lang#80965 because I didn't want the PR to
conflict with rust-lang#80914.
JohnTitor added a commit to JohnTitor/rust that referenced this pull request Apr 3, 2021
@JohnTitor
Rollup merge of rust-lang#83756 - camelid:internal-rename-doc-spotlig…
0daec04 
…ht, r=GuillaumeGomez

rustdoc: Rename internal uses of `spotlight`

I didn't make these renames in rust-lang#80965 because I didn't want the PR to
conflict with rust-lang#80914.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@CraftSpider CraftSpider CraftSpider left review comments

@LingMan LingMan LingMan left review comments

@jyn514 jyn514 jyn514 left review comments

@camelid camelid camelid requested changes

Assignees

@jyn514 jyn514

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. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue.
Projects
None yet
Milestone
1.52.0
Development

Successfully merging this pull request may close these issues.
10 participants
@GuillaumeGomez @jyn514 @camelid @bors @rust-log-analyzer @CraftSpider @rust-timer @LingMan @rust-highfive @rustbot