dbrgn / tealdeer

A very fast implementation of tldr in Rust.
https://dbrgn.github.io/tealdeer/
Apache License 2.0
4.09k stars 124 forks source link

Search pages in all platforms #279

Open marchersimon opened 2 years ago

marchersimon commented 2 years ago

Fixes #278

I don't know if the implementation is efficient, but at least it feels logical. What I've done:

It seems to work and doesn't break anything (hopefully)

niklasmohrin commented 2 years ago

Thanks! This seems like a good change, I have some comments about implementation though. Firstly, I don't think we need an efficient priority queue - these smart data structures are sometimes even slower for small inputs than just doing the straight-forward thing (see for example Vec::clear_duplicates in extensions.rs). Using a vector and just sorting it by priority is probably faster and doesn't need the dependency. Secondly, you repeat the possible platform dirs, but this should really be something like Platform::all(). This way, we don't duplicate this knowledge between multiple places.

In theory, we are doing extra IO work because we are checking the current platform and common twice, but I think this shouldn't make much of a difference (and if it does, we can fix that in another PR).

Please let us know if you need any help or have questions, note that both @dbrgn and I might take some time to respond (like with this PR)

marchersimon commented 2 years ago

Using a vector and just sorting it by priority is probably faster and doesn't need the dependency.

Good point. I'll try to use a vector for it.

In theory, we are doing extra IO work because we are checking the current platform and common twice

Actually not, because when adding a new element to a priority_queue, which does already exist, the priority of the existing element is updated.

I'll make the changes in the coming days, thanks for the feedback

marchersimon commented 2 years ago

@niklasmohrin @dbrgn I've now made the changes

marchersimon commented 2 years ago

Thanks for the extensive feedback @niklasmohrin. Unfortunately this is where my Rust knowledge ends. I haven't really worked with slices, lifetime specifiers or other Rust patters yet. I'm still learning and it could take some while 'till i get there. But if you want to, you can finish my PR.

niklasmohrin commented 1 year ago

No worries, this is what review is for :) You can take your time if you want to move forward with the PR and understand the proposed changes. Feel free to ask questions at any time, we might take some time to respond though.

if you dont want to work on it anymore at some poibt, I can also apply the review. In this case, let me know :)

matthew-e-brown commented 1 year ago

I just ran into this issue myself: I use a Windows machine for work, but spend a lot of time SSH'd into remote Linux boxes. So, things like tldr a2enmod don't appear unless I want to manually -p linux each time, or install and run tealdeer on the remote Linux box, which isn't always feasible.

I've given a quick read through this PR, and I must commend @niklasmohrin on his review style—very warm feedback for somebody who is/was new to Rust. So, @marchersimon, I'd like to add my encouragement for you to come back and move forward on this PR now that a few months have passed. How do you feel about moving forwards? It would be great to see it implemented. I have faith in you! 😄

I would of course be willing to help as well, or even take it over from you if that's what you'd like. As someone with ties to teaching, though, I would much prefer to see it be done by somebody who is/was using it as a chance to learn a new language.

Cheers. 🙂

niklasmohrin commented 1 year ago

@matthew-e-brown Due to the inactivity, I think it would be fair for you to take this PR over if you want to