Allow querying multiple platforms

[jj-style]

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.

[niklasmohrin]

Thank you for contributing! I have some change requests below, the main one is passing platforms to the methods that need it and not storing it in the Cache. This also includes handling multiple platforms in these methods instead of running them multiple times. Reviews might take a while here as @dbrgn and I are both relatively busy; feel free to ping us again here if we haven't checked in in a while or if you feel like we forgot you

[jj-style]

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]

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

[jj-style]

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

[niklasmohrin]

Thanks, looks good :)

Some of my previous comments are still open, in particular what should happen with --list, and whether we want to guarantee that we respect the order the platforms were given in. For the latter, I think we can always prioritize platforms that came first (as we already do) - so we would just need a test that covers this. @dbrgn Do you have opinions on behavior and whether we want to try to incorporate this into the spec?

[jj-style]

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

[niklasmohrin]

Thanks, looks very good now! I have some final remarks, mostly about testing, below.

There is also one comment from before that I think hasn't been addressed yet. A check should be added to make sure that when running tldr --platform linux --platform windows xyz, a page for linux should be preferred, even if xyz exists for both platforms. I think it could be tested in test_multiple_platform_command_search.

I thought about the behavior for --list some more and I think the current version is probably what we want. I am fine with the current behavior and if we ever need to change it, well, we can still change it :^)

[jj-style]

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

[niklasmohrin]

Looks good to me! Thank you for your contribution and sorry for the long wait :)

[niklasmohrin]

Sorry for the long wait! Your PR looks good and will be included in the next release. Thanks!

[dbrgn]

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]

• 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]

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]

Help text PR is here: #375