Skip to content

Fix global search - #46175

Merged
bors merged 1 commit into
rust-lang:masterfrom
GuillaumeGomez:fix-global-search
Nov 28, 2017
Merged

Fix global search#46175
bors merged 1 commit into
rust-lang:masterfrom
GuillaumeGomez:fix-global-search

Conversation

@GuillaumeGomez

@GuillaumeGomezGuillaumeGomez commented Nov 21, 2017

Copy link
Copy Markdown
Member

@GuillaumeGomezGuillaumeGomez changed the title Fix global search[WIP] Fix global searchNov 21, 2017
@GuillaumeGomez

GuillaumeGomez commented Nov 21, 2017

Copy link
Copy Markdown
MemberAuthor

Seems like it messes with generics.

@GuillaumeGomezGuillaumeGomez changed the title [WIP] Fix global searchFix global searchNov 21, 2017
@GuillaumeGomez

GuillaumeGomez commented Nov 21, 2017

Copy link
Copy Markdown
MemberAuthor

Ok, generics are working again. Added comments to avoid removing the same code again.

GuillaumeGomez
GuillaumeGomez commented Nov 22, 2017
View reviewed changes
Comment threadsrc/librustdoc/html/static/main.js Outdated

@GuillaumeGomezGuillaumeGomezNov 22, 2017

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

I'm still wondering if I should return the lowest lev distance or if I should return the average lev value... Any preference?

@QuietMisdreavusQuietMisdreavusNov 27, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

I can see how returning the average would be better, but i'm not sure how effective it will be. A user would need to search for something with multiple generics (which i'm not sure works right now? a search for Result<File, Error> returns a lot of bad lev results before any of the functions that return that type show up) before the difference will even show up. I think we can leave this as-is for now and investigate it in the future.

@GuillaumeGomezGuillaumeGomezNov 28, 2017

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

I'll leave the comment just in case then if you don't mind.

@kennytmkennytm added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Nov 22, 2017
@GuillaumeGomezGuillaumeGomez mentioned this pull request Nov 24, 2017
Closed
Havvy
Havvy reviewed Nov 25, 2017
View reviewed changes
Comment threadsrc/librustdoc/html/static/main.js Outdated

@HavvyHavvyNov 25, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Not really an issue with this PR, but I don't like how there's inconsistent var styles being used with some functions having one var per line, and others using commas like they're going out of style.

@GuillaumeGomezGuillaumeGomezNov 25, 2017

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

I grouped them by "theme". But if you want I can put each on a line.

Havvy
Havvy reviewed Nov 25, 2017
View reviewed changes
Comment threadsrc/librustdoc/html/static/main.js Outdated

@HavvyHavvyNov 25, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

What kind of obj is this?

@GuillaumeGomezGuillaumeGomezNov 25, 2017

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

The same as anywhere else in the code using searchIndex? :p How would you name it?

Comment threadsrc/librustdoc/html/static/main.js Outdated

@HavvyHavvyNov 25, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

Also not really an issue with this PR, but the isType !== true in an if thing really isn't saving you any speed of !isType.

@GuillaumeGomezGuillaumeGomezNov 25, 2017

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Don't care. In principle it's faster since the comparison is explicit.

Comment threadsrc/librustdoc/html/static/main.js Outdated

@HavvyHavvyNov 25, 2017

Copy link
Copy Markdown
Contributor

Choose a reason for hiding this comment

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

This line seems like it was left behind by accident.

@GuillaumeGomezGuillaumeGomezNov 26, 2017

Copy link
Copy Markdown
MemberAuthor

Choose a reason for hiding this comment

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

Good catch!

@bors

bors commented Nov 25, 2017

Copy link
Copy Markdown
Collaborator

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

@GuillaumeGomez

GuillaumeGomez commented Nov 26, 2017

Copy link
Copy Markdown
MemberAuthor

Rebased.

@QuietMisdreavus

QuietMisdreavus commented Nov 27, 2017

Copy link
Copy Markdown
Contributor

@bors r+

@bors

bors commented Nov 27, 2017

Copy link
Copy Markdown
Collaborator

📌 Commit 0a12198 has been approved by QuietMisdreavus

@kennytm

kennytm commented Nov 27, 2017•
edited
Loading

Copy link
Copy Markdown
Member

I suggest we backport this (and possibly #46081 also) to 1.23-beta, as search is broken on beta as well.

@QuietMisdreavus

QuietMisdreavus commented Nov 27, 2017

Copy link
Copy Markdown
Contributor

I agree - if possible, i'd rather not break the search for docs that show up on stable.

@kennytmkennytm added the beta-nominated Nominated for backporting to the compiler in the beta channel. label Nov 27, 2017
@GuillaumeGomez

GuillaumeGomez commented Nov 28, 2017

Copy link
Copy Markdown
MemberAuthor

Let's move up the priority then.

@bors: p=10

@bors

bors commented Nov 28, 2017

Copy link
Copy Markdown
Collaborator

⌛ Testing commit 0a12198 with merge 3bde5e7...

bors added a commit that referenced this pull request Nov 28, 2017
@bors
Auto merge of#46175- GuillaumeGomez:fix-global-search, r=QuietMisdr…
3bde5e7
…eavus
Fix global search
Fixes#46021.
r? @QuietMisdreavus
@bors

bors commented Nov 28, 2017

Copy link
Copy Markdown
Collaborator

☀️ Test successful - status-appveyor, status-travis
Approved by: QuietMisdreavus
Pushing 3bde5e7 to master...

@bors
bors merged commit 0a12198 into rust-lang:masterNov 28, 2017
@GuillaumeGomez
GuillaumeGomez deleted the fix-global-search branch November 28, 2017 16:55
@durkadurka mentioned this pull request Dec 20, 2017
Merged
bors added a commit that referenced this pull request Dec 23, 2017
@bors
Auto merge of#46886- durka:doc-search-backports, r=GuillaumeGomez
a1e33e1
[beta] Doc search backports
This is a backport of #46081, #46175, #46433, and #46672. They all merged cleanly but I haven't tried a build; let's see what Travis says.
These PRs fix pretty annoying issues with doc search and so I think it's important they don't slip to stable, but these PRs have *NOT* been `beta-accepted` yet.
cc @steveklabnik@GuillaumeGomez can you tag the docs team to talk about beta-acceptance?
@alexcrichton

alexcrichton commented Jan 10, 2018

Copy link
Copy Markdown
Member

Looks like we forgot to backport this to 1.23.0 (sorry about that!) so removing beta tags

@alexcrichtonalexcrichton removed the beta-nominated Nominated for backporting to the compiler in the beta channel. label Jan 10, 2018
@alexcrichton

alexcrichton commented Jan 10, 2018

Copy link
Copy Markdown
Member

Er sorry looks like this was backported in #46886

@alexcrichtonalexcrichton added the beta-accepted Accepted for backporting to the compiler in the beta channel. label Jan 10, 2018
Sign up for freeto join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

2 more reviewers
@HavvyHavvyHavvy left review comments
@QuietMisdreavusQuietMisdreavusQuietMisdreavus left review comments
Reviewers whose approvals may not affect merge requirements

Assignees

@QuietMisdreavusQuietMisdreavus

Labels

beta-acceptedAccepted for backporting to the compiler in the beta channel.S-waiting-on-reviewStatus: Awaiting review from the assignee but also interested parties.

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

6 participants

@GuillaumeGomez@bors@QuietMisdreavus@kennytm@alexcrichton@Havvy