-
Notifications
You must be signed in to change notification settings - Fork 13.5k
rustdoc search: rank aliases lower than exact matches #142653
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
base: master
Are you sure you want to change the base?
rustdoc search: rank aliases lower than exact matches #142653
Conversation
This comment has been minimized.
This comment has been minimized.
3d87e1c
to
cbf25eb
Compare
This comment has been minimized.
This comment has been minimized.
cbf25eb
to
6a8013d
Compare
This comment has been minimized.
This comment has been minimized.
6a8013d
to
07f9c38
Compare
07f9c38
to
f9bf376
Compare
rustbot has assigned @GuillaumeGomez. Use |
Some changes occurred in HTML/CSS/JS. |
let exactMatches = 0; | ||
while ( | ||
ret.others[exactMatches] !== undefined && | ||
ret.others[exactMatches].dist === 0 && | ||
ret.others[exactMatches].path_dist === 0 | ||
) { | ||
exactMatches += 1; | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I'm not sure it's the right approach: considering we plan to treat aliases the same way as all other items in the search, why not instead increase the computed distance a little (like by 0.1) to ensure it will always go after items of the same distance?
With this current code (iiuc), for each matching alias, we need to go through all results, which doesn't seem great performance wise.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
we only have to go through all the exact matches, which there typically shouldn't only be a handful of, then the loop stops.
i initially was going to try to add alias handling before the sorting, but it turns out the sorting function is also what cleans the results, and i wasn't confident enough in our typechecking yet to translate the alias results into the correct form. I was also worried about aliases missing a bunch of fields that are used in sorting.
I think it makes sense to fix this annoying issue sooner even if it has to be redone in the future, but if you prefer I refactor aliases immediately, I can do that.
Also, there's no need to meddle with the distance, we can just sort on the presence of the alias or is_alias field.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Changing the distance is simple and doesn't require any other change in the sorting, hence why I think it's the way to go.
And I'd prefer for things to be done all at once directly. The changes should be rather small so let's do it. :)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
DocSearch.ALIASES
does seem to store aliases in the standard rustdoc.Row
format, so that's not an issue, but solving this properly is semi-blocked on #142270, as that PR gives addIntoResults
an actually correct type signature.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I suppose since that PR has merge conflict I could just close it and integrate its changes into this PR.
will fix #140968