Skip to content

Sidebar trait items order#83051

Merged
bors merged 3 commits intorust-lang:masterfrom
GuillaumeGomez:sidebar-trait-items-order
Mar 24, 2021
Merged

Sidebar trait items order#83051
bors merged 3 commits intorust-lang:masterfrom
GuillaumeGomez:sidebar-trait-items-order

Conversation

@GuillaumeGomez
Copy link
Member

@GuillaumeGomez GuillaumeGomez commented Mar 12, 2021

We were actually sorting Symbol and not String, creating a completely invalid sort result. I added a test to prevent regressions.

r? @jyn514

@GuillaumeGomez GuillaumeGomez added T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. A-rustdoc-ui Area: Rustdoc UI (generated HTML) labels Mar 12, 2021
@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Mar 12, 2021
@GuillaumeGomez
Copy link
Member Author

GuillaumeGomez commented Mar 18, 2021

r? @CraftSpider

@GuillaumeGomez
Copy link
Member Author

GuillaumeGomez commented Mar 18, 2021

Ah right, they're not a reviewer. Well, in any case. :)

@CraftSpider
Copy link
Contributor

CraftSpider commented Mar 22, 2021

Now that I am a reviewer, I'll try to get to this tonight :P

@jyn514
Copy link
Member

jyn514 commented Mar 22, 2021

We were actually sorting Symbol and not String, creating a completely invalid sort result. I added a test to prevent regressions.

Why would that make it invalid? Don't Symbols sort the same as strings?

@CraftSpider
Copy link
Contributor

CraftSpider commented Mar 22, 2021 •
edited
Loading

We were actually sorting Symbol and not String, creating a completely invalid sort result. I added a test to prevent regressions.

Why would that make it invalid? Don't Symbols sort the same as strings?

Symbols derive PartialOrd and Ord, they're sorted and hashed by their SymbolIndex

@GuillaumeGomez
Copy link
Member Author

GuillaumeGomez commented Mar 22, 2021

We were actually sorting Symbol and not String, creating a completely invalid sort result. I added a test to prevent regressions.

Why would that make it invalid? Don't Symbols sort the same as strings?

Well, I guess the current situation answers your question. :)

Symbols are IDs, so when comparing two of them, you compare integers, not their content.

CraftSpider
CraftSpider approved these changes Mar 23, 2021
View reviewed changes
Copy link
Contributor

@CraftSpider CraftSpider left a comment •
edited
Loading

Choose a reason for hiding this comment

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

Looking good to me

@CraftSpider
Copy link
Contributor

CraftSpider commented Mar 23, 2021

@bors r+

@bors
Copy link
Collaborator

bors commented Mar 23, 2021

📌 Commit 15eec7c4cfb00177abad7fb72be338ef04bf1a6f has been approved by CraftSpider

@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 Mar 23, 2021
@bors
Copy link
Collaborator

bors commented Mar 23, 2021

⌛ Testing commit 15eec7c4cfb00177abad7fb72be338ef04bf1a6f with merge 85326968a127eedd34350a4c6f2995c7e48be390...

@bors
Copy link
Collaborator

bors commented Mar 23, 2021

💔 Test failed - checks-actions

@rust-log-analyzer

This comment has been minimized.

Sign in to view
@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 Mar 23, 2021
@GuillaumeGomez
Copy link
Member Author

GuillaumeGomez commented Mar 23, 2021

@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 Mar 23, 2021
@bors
Copy link
Collaborator

bors commented Mar 23, 2021

⌛ Testing commit 15eec7c4cfb00177abad7fb72be338ef04bf1a6f with merge b3397622acc8221770400ce3759957120ee67b82...

jyn514
jyn514 suggested changes Mar 23, 2021
View reviewed changes
@jyn514
Copy link
Member

jyn514 commented Mar 23, 2021

I wonder if it makes sense to change Symbols to sort by the string instead of by the index.

@GuillaumeGomez
Copy link
Member Author

GuillaumeGomez commented Mar 23, 2021

I'm not sure about that... That would perform some "hidden" operations that you might not want.

@pnkfelix
Copy link
Contributor

pnkfelix commented Mar 23, 2021

@Mark-Simulacrum asked me to retry this PR so that a higher priority stable PR preempts it. Here we go.

@pnkfelix
Copy link
Contributor

pnkfelix commented Mar 23, 2021

@bors retry

@pnkfelix
Copy link
Contributor

pnkfelix commented Mar 23, 2021

(and I guess this also buys @GuillaumeGomez some time to implement the changes suggested above! 😄 )

@rust-log-analyzer

This comment has been minimized.

Sign in to view
@GuillaumeGomez
Copy link
Member Author

GuillaumeGomez commented Mar 23, 2021

@pnkfelix Absolutely, removing it from the queue in the meantime. :)

@bors: r-

@bors bors 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-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Mar 23, 2021
@GuillaumeGomez GuillaumeGomez force-pushed the sidebar-trait-items-order branch from 15eec7c to 801ee83 Compare March 23, 2021 16:37
@GuillaumeGomez
Copy link
Member Author

GuillaumeGomez commented Mar 23, 2021

And updated! Let's go again/

@bors: r=CraftSpider,jyn514

@bors
Copy link
Collaborator

bors commented Mar 23, 2021

📌 Commit 801ee83 has been approved by CraftSpider,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 Mar 23, 2021
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this pull request Mar 23, 2021
@Dylan-DPC
Rollup merge of rust-lang#83051 - GuillaumeGomez:sidebar-trait-items-…
107e0dd 
…order, r=CraftSpider,jyn514

Sidebar trait items order

We were actually sorting `Symbol` and not `String`, creating a completely invalid sort result. I added a test to prevent regressions.

r? `@jyn514`
@Dylan-DPC-zz Dylan-DPC-zz mentioned this pull request Mar 23, 2021
Closed
@Dylan-DPC-zz Dylan-DPC-zz mentioned this pull request Mar 24, 2021
Merged
bors added a commit to rust-lang-ci/rust that referenced this pull request Mar 24, 2021
@bors
Auto merge of rust-lang#83432 - Dylan-DPC:rollup-4z5f6al, r=Dylan-DPC
db492ec 
Rollup of 9 pull requests

Successful merges:

 - rust-lang#83051 (Sidebar trait items order)
 - rust-lang#83313 (Only enable assert_dep_graph when query-dep-graph is enabled.)
 - rust-lang#83353 (Add internal io::Error::new_const to avoid allocations.)
 - rust-lang#83391 (Allow not emitting `uwtable` on Android)
 - rust-lang#83392 (Change `-W help` to display edition level.)
 - rust-lang#83393 (Codeblock tooltip position)
 - rust-lang#83399 (rustdoc: Record crate name instead of using `None`)
 - rust-lang#83405 (Slight visual improvements to warning boxes in the docs)
 - rust-lang#83415 (Remove unnecessary `Option` wrapping around `Crate.module`)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit f134ca3 into rust-lang:master Mar 24, 2021
@rustbot rustbot added this to the 1.53.0 milestone Mar 24, 2021
@GuillaumeGomez GuillaumeGomez deleted the sidebar-trait-items-order branch March 24, 2021 19:16
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

2 more reviewers

@jyn514 jyn514 jyn514 requested changes

@CraftSpider CraftSpider CraftSpider approved these changes

Reviewers whose approvals may not affect merge requirements

Assignees

@jyn514 jyn514

Labels

A-rustdoc-ui Area: Rustdoc UI (generated HTML) 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.53.0

Development

Successfully merging this pull request may close these issues.

8 participants

@GuillaumeGomez @CraftSpider @jyn514 @bors @rust-log-analyzer @pnkfelix @rust-highfive @rustbot