Allow querying multiple platforms

tealdeer-rs / tealdeer

A very fast implementation of tldr in Rust.

https://tealdeer-rs.github.io/tealdeer/

Apache License 2.0

4.17k stars 123 forks source link

Allow querying multiple platforms #300

Closed jj-style closed 1 year ago

jj-style commented 1 year ago

Fix for an old issue #45 - to allow specifying multiple platforms on the cli. Behaviour is to try get the tldr page for each platform given - so if a page doesn't exist for one platform but does for another, you still get the page displayed.

jj-style commented 1 year ago

Thank you for the code review and suggestions! I agree about passing in platform where it's needed and refactoring it out of the Cache. I'll have a crack at fixing these issues, I'm still somewhat new to rust but hopefully should be doable and will be a good learning experience!

p.s. thanks for the awesome project - it saves my bacon multiple times a day!

niklasmohrin commented 1 year ago

Feel free to ask about any problems you encounter, I will try to answer without unreasonable delays

jj-style commented 1 year ago

Happy new year! Sorry it's taken me a while to respond to your feedback. I've given it an attempt to remove the platform from the Cache and pass in &[PlatformType] on the methods that need it. It definitely feels better than looping the main program for each platform and creating multiple instances of the cache

jj-style commented 1 year ago

Thanks for the feedback! I have now implemented the suggestions you have made.

With regards to what should happen with --list when multiple platforms are provided - the current behaviour in cache.rs with the pages.sort().dedup() implies that the order the platforms are given is not important and the combined ordered set of pages should be listed. I have added a test for this behaviour however, if we decide we want to list the pages grouped in order by the platforms supplied I can make this change

jj-style commented 1 year ago

thank you for the continuous feedback and help with this. I have pushed the changes for the testing :)

dbrgn commented 8 months ago

Hi, some (very late, I know) follow-up questions @niklasmohrin @jj-style:

Was it always clear that the API should be tldr --platform linux --platform windows as opposed to something like tldr --platform linux,windows? Or was that second alternative considered?
The client specification does not mention the behavior in case of multiple pages. Niklas asked this in this comment, but I didn't see that it was addressed. Are there any other clients that support specifying multiple platforms? How does their API look like?
(Also, the help seems confusing to me now, because it mixes a singular keyword with a plural parameter name. But I can fix that in a separate PR.)

niklasmohrin commented 8 months ago

I dont remember exactly, this is just the default behavior of clap for this. I think I slightly prefer the current version, because it passes multiple platforms via multiple arguments and a parsing error could point directly to which platform is invalid.
I thought that the tests assert a reasonable behavior - platforms are checked in the order they are passed - but I dont fully remember and would have to dig in again. If the spec decides something else, we can change it again
Agree, this could be improved. Maybe a note that it can be passes multiple times would be better

jj-style commented 8 months ago

I did not check if or how other clients support multiple platforms, sorry. I agree with @niklasmohrin about using the default clap behaviour but happy to see if there are other ways of doing it in clap that provide a more concise interface.

Happy to update the help text in another PR either way!

dbrgn commented 2 months ago

Help text PR is here: #375