Conversation
|
I'm not entirely sure why it fails but I'm going to close this for now. |
There was a problem hiding this comment.
Hiya, not a maintainer or the owner but I noticed you submitted this PR optimising code I contributed. It's a really quick fix for you to apply then hopefully we can get this merged!
When I originally wrote the extension-checking part of the program I was still pretty novice-level at Rust, so I appreciate you coming back and improving on my work
src/lib.rs
Outdated
|
|
||
| // Originally taken from: | ||
| // https://github.com/nushell/nushell/blob/93e8f6c05e1e1187d5b674d6b633deb839c84899/crates/nu-cli/src/completion/command.rs#L64-L74 | ||
| let pathext = pathext |
There was a problem hiding this comment.
This is the only part that's not quite right. You removed the original return statement I had and haven't provided it again, meaning the program always falls back to the old logic (lines 132 onwards of your branch)
diff --git a/src/lib.rs b/src/lib.rs
index 8b1c664..dfc71e5 100644
--- a/src/lib.rs
+++ b/src/lib.rs
@@ -117,7 +117,7 @@ mod windows {
// Originally taken from:
// https://github.com/nushell/nushell/blob/93e8f6c05e1e1187d5b674d6b633deb839c84899/crates/nu-cli/src/completion/command.rs#L64-L74
- let pathext = pathext
+ return pathext
.to_string_lossy()
.split(';')
// Filter out empty tokens and ';' at the endThis'll make tests pass and reduce allocations as you intended
There was a problem hiding this comment.
Oh, hello! Alright, let's try that.
|
Let's get this merged! @fitzgen If you wanted help maintaining the project, I'd be happy to offer my services :) |
Hello, I've taken a closer look at the Windows implementation of
is_executableand I think this can be optimized.First, I believe theto_string_lossycan simply be removed from bothextensionandpathextand shouldn't affect the comparison but I could be wrong on that one so please test this if you are able to.As for the
.map(|ext| ext[1..].to_string()), I was able to remove theto_stringby simply merging the cut-off into theany. Thecollectis unnecessary and theanyiteration can simply be done immediately instead of collecting into aVecfirst.