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.

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: sort deprecated items lower in search #107629

Merged
merged 1 commit into from
Mar 11, 2023
Merged

rustdoc: sort deprecated items lower in search #107629

merged 1 commit into from
Mar 11, 2023

Conversation

pitaj
Copy link
Contributor

@pitaj pitaj commented Feb 3, 2023

closes #98759

Screenshots

i32::MAX show sup above std::i32::MAX and core::i32::MAX
image
If just searching for min, the deprecated results show up far below other things:
image
one page later
image

And, as you can see, the "Deprecation planned" message shows up in the search results. The same is true for fully-deprecated items like mem::uninitialized:
image

Edit: the deprecation message change was removed from this PR. Only the sorting is changed.

@rustbot
Copy link
Collaborator

rustbot commented Feb 3, 2023

r? @jsha

(rustbot has picked a reviewer for you, use r? to override)

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. labels Feb 3, 2023
@rustbot
Copy link
Collaborator

rustbot commented Feb 3, 2023

Some changes occurred in HTML/CSS/JS.

cc @GuillaumeGomez, @Folyd, @jsha

@GuillaumeGomez
Copy link
Member

Thanks for the PR but no decision was reached on the issue so it might be a bit too early. Don't hesitate to bring it up on the #t-rustdoc channel on our zulip instance.

@tgross35
Copy link
Contributor

tgross35 commented Feb 3, 2023

no decision was reached on the issue so it might be a bit too early

Fwiw, I think this would still be nice outside of #107587. Probably moreso in crates, which tend to have a lot more deprecations than std, but there are still a few cases floating around there as well.

image

A deprecated tag in the search results might also be nice similar to #107568, but this might run into the same issues as that. (this might be the case already, I haven't had a chance to build this branch and try it out)

@pitaj

This comment was marked as outdated.

@pitaj
Copy link
Contributor Author

pitaj commented Feb 23, 2023

Okay it seems like this isn't blocked on anything. Should be able to move forward with an FCP here once the review is complete. Zulip discussion for reference

@jsha mind taking a look?

Copy link
Contributor

@jsha jsha left a comment

Choose a reason for hiding this comment

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

Thanks for working on this! Sorting the deprecated items lower is good; I think we should not add the "Deprecation planned" / "Deprecated" right there in the search results, because it actually draws more attention to the less important items, and because it uses up valuable space.

Implementation-wise, I think the priority where you've placed the sorting-level tweak for deprecation is good. Better string matches should take priority over the deprecation bump. 👍🏻 One thing I notice, looking at these, is that there's a bump for "shorter item name." In theory that should make char::is_ascii rank above std::ascii::AsciiExt::is_ascii even without this PR. But that's not the case. I think there's a separate issue - the "same crate" bump is giving std::ascii::AsciiExt::is_ascii priority when searching in std. We should fix that separately - for instance, by treating all primitives as "same crate" relative to std, core, and alloc.

For storage in the JSON search index, this PR includes a 0 for all non-deprecated items. That's not very efficient for the typical case where most items are non-deprecated. How about instead storing a list of item indexes that are deprecated? And could you include a before/after size of the search index for the std/core/alloc docs (i.e. the output from ./x.py doc)?

@pitaj
Copy link
Contributor Author

pitaj commented Mar 4, 2023

I think we should not add the "Deprecation planned" / "Deprecated" right there in the search results, because it actually draws more attention to the less important items, and because it uses up valuable space.

Hmm. I think having that information in the search results is quite useful and would save people time. What about using a text effect such as strikethrough, color, or opacity?

@pitaj
Copy link
Contributor Author

pitaj commented Mar 7, 2023

@jsha okay I've remove the deprecation message from the search results. I've also switched the deprecated and "full path" columns to a sparse style, reducing the search index size by 4%

search-index.js size [B]
before 3893476
after 3725618

@pitaj
Copy link
Contributor Author

pitaj commented Mar 7, 2023

One thing I notice, looking at these, is that there's a bump for "shorter item name." In theory that should make char::is_ascii rank above std::ascii::AsciiExt::is_ascii even without this PR. But that's not the case. I think there's a separate issue - the "same crate" bump is giving std::ascii::AsciiExt::is_ascii priority when searching in std. We should fix that separately - for instance, by treating all primitives as "same crate" relative to std, core, and alloc.

I also looked into this a little. It's not that the primitives don't count as being in the preferredCrate, they show up as belonging to std. It's actually that there is no sorting on the item path length, only on the "word" length - which appears to be the portion of the path that matched. We could add a sort on xxx.item.path.length, but it's not clear from my experiment locally whether it consistently produces better results.

src/librustdoc/html/static/js/search.js Outdated Show resolved Hide resolved
src/librustdoc/html/static/js/search.js Outdated Show resolved Hide resolved
Copy link
Contributor

@jsha jsha left a comment

Choose a reason for hiding this comment

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

Looks great, just one comment fix and we're ready to go.

reducing the search index size by 4%

oh, very nice!

And thanks for the research on why the "shorter name" match wasn't sufficient to rank the primitive associated items higher. That's good to know.

src/librustdoc/html/static/js/search.js Show resolved Hide resolved
@jsha
Copy link
Contributor

jsha commented Mar 10, 2023

Looks great! Thanks. Want to squash the commits? r=me with that done.

serialize `q` (`itemPaths`) sparsely
overall 4% reduction in search index size
@pitaj
Copy link
Contributor Author

pitaj commented Mar 10, 2023

Squash complete

r? @jsha

@rustbot
Copy link
Collaborator

rustbot commented Mar 10, 2023

Could not assign reviewer from: jsha.
User(s) jsha are either the PR author or are already assigned, and there are no other candidates.
Use r? to specify someone else to assign.

@notriddle
Copy link
Contributor

@bors r=jsha rollup

@bors
Copy link
Contributor

bors commented Mar 10, 2023

📌 Commit d2e4b59 has been approved by jsha

It is now in the queue for this repository.

@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 10, 2023
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Mar 11, 2023
…=jsha

rustdoc: sort deprecated items lower in search

closes rust-lang#98759

### Screenshots

`i32::MAX` show sup above `std::i32::MAX` and `core::i32::MAX`
![image](https://user-images.githubusercontent.com/803701/216725619-40afb7b0-e984-4a2e-ab5b-a95b24736b0e.png)
If just searching for `min`, the deprecated results show up far below other things:
![image](https://user-images.githubusercontent.com/803701/216725672-e4325d37-9bfe-47eb-a1fe-0e57092aa811.png)
one page later
![image](https://user-images.githubusercontent.com/803701/216725932-cd1c4a42-d527-44fb-a4ab-5a6d243659cc.png)

~~And, as you can see, the "Deprecation planned" message shows up in the search results. The same is true for fully-deprecated items like `mem::uninitialized`:
![image](https://user-images.githubusercontent.com/803701/216726268-1657e77a-563f-45a0-85a7-3a0cf4d66d6f.png)~~

Edit: the deprecation message change was removed from this PR. Only the sorting is changed.
bors added a commit to rust-lang-ci/rust that referenced this pull request Mar 11, 2023
…iaskrgr

Rollup of 9 pull requests

Successful merges:

 - rust-lang#106276 (Fix `vec_deque::Drain` FIXME)
 - rust-lang#107629 (rustdoc: sort deprecated items lower in search)
 - rust-lang#108711 (Add note when matching token with nonterminal)
 - rust-lang#108757 (rustdoc: Migrate `document_item_info` to Askama)
 - rust-lang#108784 (rustdoc: Migrate sidebar rendering to Askama)
 - rust-lang#108927 (Move __thread_local_inner to sys)
 - rust-lang#108949 (Honor current target when checking conditional compilation values)
 - rust-lang#108950 (Directly construct Inherited in typeck.)
 - rust-lang#108988 (rustdoc: Don't crash on `crate` references in blocks)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 8596751 into rust-lang:master Mar 11, 2023
@rustbot rustbot added this to the 1.70.0 milestone Mar 11, 2023
@jsha
Copy link
Contributor

jsha commented Aug 25, 2023

A friend recently pointed out that since this PR, the comment in search.js (which they found very helpful) has gone stale:

/**
* The raw search data for a given crate. `n`, `t`, `d`, and `q`, `i`, and `f`
* are arrays with the same length. n[i] contains the name of an item.
* t[i] contains the type of that item (as a string of characters that represent an
* offset in `itemTypes`). d[i] contains the description of that item.
*
* q[i] contains the full path of the item, or an empty string indicating
* "same as q[i-1]".
*
* i[i] contains an item's parent, usually a module. For compactness,
* it is a set of indexes into the `p` array.
*
* f[i] contains function signatures, or `0` if the item isn't a function.
* Functions are themselves encoded as arrays. The first item is a list of
* types representing the function's inputs, and the second list item is a list
* of types representing the function's output. Tuples are flattened.
* Types are also represented as arrays; the first item is an index into the `p`
* array, while the second is a list of types representing any generic parameters.
*
* `a` defines aliases with an Array of pairs: [name, offset], where `offset`
* points into the n/t/d/q/i/f arrays.
*
* `doc` contains the description of the crate.
*
* `p` is a list of path/type pairs. It is used for parents and function parameters.
*
* @type {{
* doc: string,
* a: Object,
* n: Array<string>,
* t: String,
* d: Array<string>,
* q: Array<[Number, string]>,
* i: Array<Number>,
* f: Array<RawFunctionSearchType>,
* p: Array<Object>,
* c: Array<Number>
* }}

@pitaj would you be willing to send another PR fixing the comment to reflect the updated format?

@pitaj
Copy link
Contributor Author

pitaj commented Sep 3, 2023

@jsha sorry about that, comment is fixed in #115490

GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this pull request Sep 3, 2023
…GuillaumeGomez

rustdoc: update comment in search.js for rust-lang#107629

Addressing rust-lang#107629 (comment)

r? `@jsha`
bors added a commit to rust-lang-ci/rust that referenced this pull request Sep 3, 2023
…llaumeGomez

Rollup of 3 pull requests

Successful merges:

 - rust-lang#115478 (Emit unused doc comment warnings for pat and expr fields)
 - rust-lang#115490 (rustdoc: update comment in search.js for rust-lang#107629)
 - rust-lang#115503 (Migrate GUI colors test to original CSS color format)

r? `@ghost`
`@rustbot` modify labels: rollup
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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
Development

Successfully merging this pull request may close these issues.

Rustdoc reduce search rank for deprecated items
8 participants