Add common platform to CLI

[beatbrot]

Fixes #323

[niklasmohrin]

Thanks for the PR! As "common" is the fallback choice for other platforms, can you change it so that we don't have that behavior hardcoded in the code of Cache and instead include the new Platform::Common in the list of searched platforms in main.rs?

[beatbrot]

@niklasmohrin Done :)

[niklasmohrin]

As Rust 1.84 was released today and includes new linting rules, the CI now also has a failure with clippy on code that is unrelated to your PR. Feel free to ignore the failure for that check

[niklasmohrin]

We still have an occurrence of "common" in the code for listing all available pages which should be removed. Once this is done, one of our tests will fail that asserts that using tldr --platform linux actually checks both "linux" and "common" (for printing pages, as well as for listing them). So we should make sure to append "common" to the list of considered platforms here also in the case that args.platforms is Some(_). It's fine if we clone args.platforms for this.

(the comments for listing pages also include mentions of the "common" platform, which should then be removed)

[beatbrot]

Fair point :) I hope I found all special handlings now!

[niklasmohrin]

The behavior for --list is correct now 👍 I had another implementation in mind though, sorry for not making this clear immediately!

I think that now that we have the Common platform, we can move the "we will also check the common directory for pages" logic out of Cache entirely - this is nice, because the cache shouldn't care about the choice of platforms, only about how to find pages for the given name, platform, etc.

Thus, no method of Cache should make an exception for "common", it should just be treated like any other platform. The fact that we are still checking the "common" platform is business logic, it fits a lot better in main. I am imagining to replace the code around line 285 with something like

let fallback_platforms = ...;
let chosen_platforms = args.platforms.clone();
let platforms = chosen_platforms.something();

such that in the end platforms is either a referencing fallback_platforms or chosen_platforms and chosen_platforms has Common appended, if it was not already present.

---

This would also change the behavior for tldr --platform linux tar - right now, this would check only "linux", but the behavior of tealdeer 1.7 is to check both "linux" and "common". It seems that we are missing a test case for this, but it is definitely the expected behavior for the current version. If you are eager, you could also add a testcase for this in lib/tests.rs around the other tests checking the --platform lookup behavior (like test_multiple_platform_command_search_not_found and test_multiple_platform_command_search).

Sorry that I am inflating this PR so much, if you want to hand it off at any point please let me know. Otherwise, I am very happy to keep working on this together! :)

[beatbrot]

Nah its fine as long as you can live with a few days interruption here and there :)

Thank you very much for your thorough review! I hope I got it this time :)

[niklasmohrin]

Oh yes, I might also not be able to do thorough reviews for longer times if other work is piling up, so no worries

[niklasmohrin]

Everything looks good to me, thanks!