Some improvements. #951
Some improvements. #951
Conversation
|
Hey @Shuraken007 I didn't see this over the weekend but this is great! I'll take a look in the next couple days hopefully and either leave some notes. I appreciate the PR! I don't use devicon-lookup as much as I used to so glad others are still getting value out of it! |
improve shortening made file-name white colour
|
Btw, as you see - next commit appeared.
|
src/devicon_lookup/color.rs
Outdated
| f if self.is_temp(f) => Fixed(244).normal(), | ||
| f if self.is_immediate(f) => Fixed(1).bold().underline(), | ||
| f if self.is_image(f) => Fixed(37).normal(), | ||
| f if self.is_video(f) => Fixed(135).normal(), | ||
| f if self.is_music(f) => Fixed(92).normal(), | ||
| f if self.is_lossless(f) => Fixed(93).normal(), | ||
| f if self.is_crypto(f) => Fixed(109).normal(), | ||
| f if self.is_document(f) => Fixed(187).normal(), | ||
| f if self.is_compressed(f) => Red.normal(), | ||
| f if self.is_compiled(f) => Fixed(137).normal(), | ||
| f if self.is_pretty_data(f) => Fixed(178).normal(), | ||
| f if self.is_script(f) => Fixed(173).normal(), | ||
| f if self.is_config(f) => Fixed(65).normal(), | ||
| f if self.is_vim(f) => Fixed(71).normal(), | ||
| f if self.is_language(f) => Fixed(75).normal(), |
There was a problem hiding this comment.
Woah, I'm not sure I've seen this style of match with if self inside in the wild.
I really like it though!
|
Ok @Shuraken007 this is super awesome!! Sorry it took me awhile to take a look! But I looked at the code and I don't have many comments to leave this is awesome! I'm happy to take a closer look at the data and see if we can weave references in more if you want. Are the For testing did you have any thoughts? It looks like one of the reasons the tests are failing is that the output includes a leading slash by default now. But if you like the new format, I'd be happy just updating the tests with the new format and icons and calling it a day! And/or change any up if you think something else could be better. And for the And to wrap up, do you want commit access here? This PR is awesome! And like I said before I'm not using Oh and one more thing! Just since you mentioned being new to Rust, I wanted to mention Thanks again for this PR, it really is great! |
simplified File with regex
The most change, I suppose - you should add '-s' / '--substitute' flag, when you want to save extra things |
|
@coreyja , I make simple prototipe with |
|
Hey @Shuraken007 thanks for giving that prototype a try! Thanks for taking a stab at the reference optimizations! I definitely can try and help out with the borrow errors! I'll try to take a look this week, but there is a chance I won't get to it before the weekend. Just wanted to give you an timeline so you weren't waiting for me lol |
|
@coreyja, got it. It's still 3 times slower, than your main branch. |
…mponents()' iterator
… plus we weren't adding the right / back to the end
… of making an iter here
…ctly This refactor is mostly so that we don't need to create instances of `FileExtensions` This struct is more just a collection of functions, so it didn't make sense to create an instance of it. I might even remove the struct entirely in the future, and just have these be free functions that live in a module
…m for the evening at least
|
OK @Shuraken007! I'm sitting down to take a look at this PR. I think I'm going to do this "Lab Notebook" style, where I'm just going to record basically everything I am doing here so that hopefully you can follow along with! To start off I want to run the existing Benchmarks to see what the Initial Benchmarks
|
|
@coreyja About benchmarks. I tested on my home directory. it's about 200k files. and without any precise measures - I saw, that main branch takes smth like 1.5-2 sec, and forkBranch around 5-6. Well, I made 3 folders with versions:
pub fn print_with_symbol(&self, args: &Args) {
let s = format!("{}\n", self.original);
return write_to_stdout(s.as_bytes());Created simple bash script for measure time. #!/bin/bash
run_devicon_n_times() {
name=$1
times=$2
for i in $(seq $times); do
cat ~/tmp/devicon/file_list | eval "~/p/src/${name}/target/release/devicon-lookup" > /dev/null
done
}
mkdir -p ~/tmp/devicon
fd . ~ --hidden > ~/tmp/devicon/file_list
echo 'total_files: ' $(cat ~/tmp/devicon/file_list | wc -l)
echo
echo running 'devicon_fork':
time run_devicon_n_times devicon-lookup 10
echo
echo running 'devicon_refactor':
time run_devicon_n_times devicon_refactor 10
echo
echo running 'devicon_orig':
time run_devicon_n_times devicon_orig 10So, result is fun, before refactoring it takes x6 times. ❯ ./test
total_files: 219919
running devicon_fork:
real 0m12.232s
user 0m11.322s
sys 0m1.102s
running devicon_refactor:
real 0m2.543s
user 0m1.597s
sys 0m1.080s
running devicon_orig:
real 0m2.430s
user 0m1.668s
sys 0m0.896sWith calculating icon / extension, e.t.c. ❯ ./test
total_files: 219920
running devicon_fork:
real 0m13.753s
user 0m12.652s
sys 0m1.299s
running devicon_refactor:
real 0m5.074s
user 0m4.171s
sys 0m1.055s
running devicon_orig:
real 0m2.801s
user 0m1.859s
sys 0m1.087sI tested, what gives this slowness, it's regex. Also I played around PathBuf, added calculations for time I tried PathBuf, when started forkBranch. Founded, that it goes to file system on checking |
Folders should ends with `/` Refactored short_path
|
Also, thanks a lot for reviewing. |
Ya great point I really should have used Github Reviews, and looking back not really sure why I decided not to. Thanks for the callout! And thanks for doing some more benchmarking! That makes sense that its regex that is slow! regex'es are fast in general but doing them in a loop definitely can add up quick!
Haha love that you ran into this problem too! |
|
Hey @Shuraken007 sorry for the long delay here! But thanks again for this awesome PR! I'm going to merge it in now, and should have a new version made this afternoon that include these changes! Likely as a Thanks again!! |
|
Hi @Shuraken007 and @coreyja .. I'm trying the new release and getting the following error: (also even though I'm listing directories the folder icon is not shown; is there an extra setting needed?) Another example: if I run z or zoxide i would expect the items in the list also to be folder icons but this doesn't happen: |
|
Hey @petobens thanks for the bug report! Taking a look now Looking into the unwrap error first, and then I'll look into the dir icons |
|
@petobens this should be fixed in If it doesn't fix both those things definitely let me know! And we are finally leaning on |
|
Hi! Thanks for the quick turn around. Running On the other hand. If I run |
|
Hey @petobens! Thanks again for this report sorry the Thanks for all your help and reports! 🙏 |
|
Great!Thanks |
|
Ok @petobens! Give I also got a version working with zoxide query --list --score | devicon-lookup -r '\d +(.*)$' -sThe 
|
|
Works great! Thank you :) |
Hello. I added some extra staff from exa (https://github.com/ogham/exa) .
Specific icons + colors.
.../dir/Alas I'm new at rust. Probably do some things not optimal, espetially at creating struct File (had to copy data, no references).
I havn't touch tests.
Devicons works x2 times slower probably.
But still 3 seconds for 200k+ files vs 1 second without devicon. Suites for me.
New options:
Examples:
no additional options
color icons
nameshort
dirshort1
dirshort2
