Skip to content

Search fixes #45617

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
Merged
merged 5 commits into from
Nov 4, 2017
Merged

Search fixes #45617

merged 5 commits into from
Nov 4, 2017

Conversation

@GuillaumeGomez
Copy link
Member

@GuillaumeGomez GuillaumeGomez commented Oct 29, 2017

@kennytm kennytm added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Oct 29, 2017
kennytm
kennytm reviewed Oct 29, 2017
View reviewed changes
src/librustdoc/html/static/main.js Outdated
Copy link
Member

@kennytm kennytm Oct 29, 2017

Choose a reason for hiding this comment

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

wat.

I would have used indexOf/lastIndexOf to extract the substring instead of the split.slice.join.split.slice.join

function extractGenerics(val) {
    var values = val.substring(val.indexOf('<')+1, val.lastIndexOf('>'));
    return values.split(/\s*,\s*/);
}

Copy link
Member Author

@GuillaumeGomez GuillaumeGomez Oct 31, 2017

Choose a reason for hiding this comment

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

Way better, thanks!

src/librustdoc/html/static/main.js Outdated
Copy link
Member

@kennytm kennytm Oct 29, 2017

Choose a reason for hiding this comment

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

Do you mean to use name here? The check above says name won't contain any '<'...

(If this is expected why not just write tmp_valu = name.split(/\s*,\s*/)...)

Copy link
Member Author

@GuillaumeGomez GuillaumeGomez Oct 31, 2017

Choose a reason for hiding this comment

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

Indeed... It should have been !== -1.

Copy link
Member Author

@GuillaumeGomez GuillaumeGomez Oct 31, 2017

Choose a reason for hiding this comment

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

Or maybe not... Trying to understand what I was trying to do...

Copy link
Member Author

@GuillaumeGomez GuillaumeGomez Oct 31, 2017

Choose a reason for hiding this comment

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

No I was right: it should have been !== -1!

Copy link
Contributor

@QuietMisdreavus QuietMisdreavus Oct 31, 2017

Choose a reason for hiding this comment

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

Wait a sec, this is ripping out the contents of the generics if both the type and the search query contain them? Am i reading this correctly? It seems like we should do that if isWrapper === false instead.

src/librustdoc/html/static/main.js Outdated
Copy link
Member

@kennytm kennytm Oct 29, 2017

Choose a reason for hiding this comment

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

ditto

killercup
killercup reviewed Oct 29, 2017
View reviewed changes
src/librustdoc/html/static/main.js Outdated
Copy link
Contributor

@killercup killercup Oct 29, 2017

Choose a reason for hiding this comment

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

Why do you define this on Object (of all things) and not just as a free function?

Copy link
Member Author

@GuillaumeGomez GuillaumeGomez Oct 30, 2017

Choose a reason for hiding this comment

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

I preferred it as is but I can make a free function if you prefer.

Copy link
Contributor

@killercup killercup Oct 30, 2017

Choose a reason for hiding this comment

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

Yeah, I don't think adding a static method for this on a core, well, Object is the way to go. This function probably shouldn't escape the scope of this file anyway.

QuietMisdreavus
QuietMisdreavus reviewed Oct 31, 2017
View reviewed changes
Copy link
Contributor

@QuietMisdreavus QuietMisdreavus left a comment

Choose a reason for hiding this comment

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

I'm way shaky on everything that's going on in the last commit - it seems like the code that checks through generics should be thrown out since that needs to be in a separate PR anyway. (It's also unrelated to the linked issue.)

src/librustdoc/html/static/main.js Outdated
Copy link
Contributor

@QuietMisdreavus QuietMisdreavus Oct 31, 2017

Choose a reason for hiding this comment

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

Would it be prudent to also include the "base name" in this? It looks like this throws away the "Result" in Result<File, Error>. From how this is used it seems like it wouldn't be included in Levenstein comparisons because of that.

Copy link
Member Author

@GuillaumeGomez GuillaumeGomez Oct 31, 2017

Choose a reason for hiding this comment

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

That's the point. :)

src/librustdoc/html/static/main.js Outdated
Copy link
Contributor

@QuietMisdreavus QuietMisdreavus Oct 31, 2017

Choose a reason for hiding this comment

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

Wait a sec, this is ripping out the contents of the generics if both the type and the search query contain them? Am i reading this correctly? It seems like we should do that if isWrapper === false instead.

@GuillaumeGomez GuillaumeGomez force-pushed the search-fixes branch 2 times, most recently from 6ed3fd5 to 630957e Compare October 31, 2017 22:27
@GuillaumeGomez GuillaumeGomez mentioned this pull request Oct 31, 2017
Merged
@GuillaumeGomez
Copy link
Member Author

GuillaumeGomez commented Nov 1, 2017

Removed the genericity part. I'll put it directly in #45673.

@QuietMisdreavus
Copy link
Contributor

QuietMisdreavus commented Nov 3, 2017

Thanks! This looks good to go now.

@bors r+

@bors
Copy link
Collaborator

bors commented Nov 3, 2017

📌 Commit ee7e372 has been approved by QuietMisdreavus

@bors
Copy link
Collaborator

bors commented Nov 4, 2017

⌛ Testing commit ee7e372 with merge dcd343b...

bors added a commit that referenced this pull request Nov 4, 2017
@bors
Auto merge of #45617 - GuillaumeGomez:search-fixes, r=QuietMisdreavus
dcd343b 
Search fixes

Fixes #45608.

r? @QuietMisdreavus
@bors
Copy link
Collaborator

bors commented Nov 4, 2017

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

@bors bors merged commit ee7e372 into rust-lang:master Nov 4, 2017
@GuillaumeGomez GuillaumeGomez deleted the search-fixes branch November 4, 2017 15:40
bors added a commit that referenced this pull request Nov 13, 2017
@bors
Auto merge of #45673 - GuillaumeGomez:rustdoc-type-search-generic, r=…
c8c1424 
…QuietMisdreavus

Search over generic types in docs

This is what I was talking about @QuietMisdreavus. Now we have generics.

Waiting for #45617 to get merged.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@kennytm kennytm kennytm left review comments

+2 more reviewers

@killercup killercup killercup left review comments

@QuietMisdreavus QuietMisdreavus QuietMisdreavus left review comments

Reviewers whose approvals may not affect merge requirements

Assignees

@QuietMisdreavus QuietMisdreavus

Labels

S-waiting-on-review Status: 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.

5 participants

@GuillaumeGomez @QuietMisdreavus @bors @killercup @kennytm