Uh oh!
There was an error while loading. Please reload this page.
Fix path search - #46081
Conversation
7d0d016 to
8da48f2CompareQuietMisdreavus
commented
Nov 18, 2017
This changes the behavior when searching for paths, but it doesn't quite fix the issue? Your PR
Current nightly
Current stable
|
GuillaumeGomez
commented
Nov 18, 2017
Oh damn. Didn't take into account cases where the path doesn't start by the given search... I'll update it. |
8da48f2 to
ad6324fCompareGuillaumeGomez
commented
Nov 19, 2017
Ok fixed! The changed line is this one. |
GuillaumeGomez
commented
Nov 19, 2017
After thinking about it once again (and a few tests), I found my solution not good enough so I strongly improved the |
There was a problem hiding this comment.
Slice returns a new array, so this is a no-op. Did you want to .splice() instead? From the removed code side, it looks like the answer is yes.
More idiomatically, you can also push those that raen't the empty string.
There was a problem hiding this comment.
> x = [1,2,3];
(3) [1, 2, 3]
> x.splice(0, 1)
[1]
> x
(2) [2, 3]
So no, it clearly removes an entry from the array.
There was a problem hiding this comment.
Oh damn, you're absolutely right. Didn't see the missing 'p'!
9ae3a82 to
c00eaa9CompareGuillaumeGomez
commented
Nov 20, 2017
Fixed. |
Havvy
left a comment
There was a problem hiding this comment.
Just checking for basic JS stuff, and no other basic JS mistakes found.
| return MAX_LEV_DISTANCE + 1; | ||
| } | ||
| for (var i = 0; i < path.length; ++i) { | ||
| if (i + startsWith.length > path.length) { |
There was a problem hiding this comment.
You can use the condition used in the if statement as the for loop condition (when inverted), can't you?
There was a problem hiding this comment.
Absolutely. I just have a personal preference to keep loop conditions as simple as possible. If someone else thinks it's a good idea, then I'll just update it.
| } | ||
| lev_total += lev; | ||
| } | ||
| if (aborted === false) { |
There was a problem hiding this comment.
Isn't it more common to check it like this: !aborted
There was a problem hiding this comment.
It's slower. '!' tests (from memory): length !== 0, 0, null, undefined. So clearly, it's way better in js to make the comparison like this.
There was a problem hiding this comment.
Didn't know that. Thanks for explaining it.
There was a problem hiding this comment.
Except you can generally rely on the JIT to figure out that aborted can only contain a boolean.
There was a problem hiding this comment.
I did too much rust, I love being explicit. 😛
QuietMisdreavus
commented
Nov 25, 2017
@bors r+ |
bors
commented
Nov 25, 2017
📌 Commit c00eaa9 has been approved by |
bors
commented
Nov 25, 2017
Fix path search Fixes#46015. r? @QuietMisdreavus
bors
commented
Nov 25, 2017
☀️ Test successful - status-appveyor, status-travis |
[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
commented
Jan 10, 2018
Looks like we forgot to backport this to 1.23.0 (sorry about that!) so removing beta tags |
alexcrichton
commented
Jan 10, 2018
Er sorry, looks like this was backported in #46886 |
Fixes#46015.
r? @QuietMisdreavus