Support platform type "all"

[dbrgn]

This is the third part of #209.

The goal is supporting the special all platform that results in pages
for all platforms being listed when calling --list. It's part of the
tldr client specification.

However, All should not be a variant of the PlatformType enum,
because Current isn't a PlatformType either. Thus, we accept the
string all but convert it into the current platform when parsing.

For consistency, the same is done when no platform is specified, by
introducing yet another possible value current which is used by
default. This way, we get rid of the Option.

Due to limitations in clap, we had to remove the requires = "command"
requirement, however that's actually a good thing because --list does
not require a command either, but supports the --platform parameter.

To simplify handling of os / platform arguments, a conflict between
--platform and --os was introduced.

[niklasmohrin]

Here I am, doing what I do best: Proposing architectural changes 😅

[niklasmohrin]

Hm, that doesn't feel right, I don't like it :/. Couldn't we model it so there is an enum Platform (or PlatformType) which has linux, windows, etc. and then there is an enum PlatformStrategy { All, Priority(Platform) }? find_page could then reject PlatformStrategy::All or just stick to the "normal" Platform as an argument.

This ofc complicates parsing a bit, but also makes it closer to the actual spec again: We change the type for --platform to PlatformStrategy and make it optional again. What do you think?

[dbrgn]

We could, but then where do you do the ::current() lookup?

I'm also not fond of the hack I used, but I wanted to avoid adding a wrapper type. However, if we'd add a wrapper type, why not a struct instead of an enum?

struct PlatformStrategy {
    platform: PlatformType,
    list_all: bool,
}

[niklasmohrin]

We could, but then where do you do the ::current() lookup?

I would keep PlatformType::current as it is on main right now, just returning the correct variant.

I'm also not fond of the hack I used, but I wanted to avoid adding a wrapper type. However, if we'd add a wrapper type, why not a struct instead of an enum?

Do we want to allow --platform all when actually looking for a page to display, or just for --list? I think the latter. Then, an enum is right. The code for --list accepts both variants (by having its argument be PlatformStrategy), whereas the normal path only takes a PlatformType as an argument. In both cases, if we allowed specifying the list_all flag and a specific platform, we need to make choices about prioritization, which is just complication and not covered in the spec either. Or is there any benefit in still considering a specific platform even if --platform all was passed? I could totally be missing something :D

[dbrgn]

Do we want to allow --platform all when actually looking for a page to display, or just for --list? I think the latter.

Yes, at least that's what the spec says.

Then, an enum is right. The code for --list accepts both variants (by having its argument be PlatformStrategy)

But the PlatformStrategy doesn't contain a "resolved" platform type when the variant is All.

The type "all" is not something that conflicts with a concrete platform type. It behaves like the "current" platform type (which is resolved in main). The only difference is that when listing the pages, all platforms are included. Otherwise it acts like "current".

The listing logic lives within the cache code. The cache code must be initialized with a platform type. Thus, if we only pass in enum PlatformStrategy { All, Priority(Platform) }, every part of the caching code that deals with the platform type must match on the strategy, and handle the error case All (because that doesn't contain a concrete platform type). In contrast, in the current implementation, everything just works as before.

I see two ways to implement the "all" platform type cleanly without the { all: bool } enum hack:

• A: The current approach
• B: A wrapper type like this:

struct PlatformStrategy {
    platform: PlatformType,
    list_all: bool,
}

The --platform parameter type would be PlatformStrategy. The cache would be initialized with the PlatformStrategy instance (because that's where listing the pages is done), while every other part of the code that deals with the platform type would take a PlatformType as before.

[niklasmohrin]

Ah, we are designing two different interfaces right now 😅

Yes, at least that's what the spec says.

But where? This is what the requirement in our issue says, but in the client spec I can only find it mentioned once in the table where the flags are listed. I would think about --list as a subcommand rather than an addition to the main use case. It kind of is, the --list option triggers an exit(0) before we look into our current pages dir, so why bother resolving the platform for looking up pages? Am I not seeing something?

But the PlatformStrategy doesn't contain a "resolved" platform type when the variant is All.

... which matches what I believe to be the correct interpretation

[dbrgn]

Btw, I'll rewrite it for variant B, I agree that it's cleaner. And then we have actual code to discuss.

[dbrgn]

Pushed 🙂

[niklasmohrin]

Okay, I can see your point. I feel like both interpretations make sense, so I guess we should ask upstream for clarification? (I am not really confortable to commit to your suggestion with the risk of having a breaking change later, but my suggestion could likely be too complicated.) Should I open an issue?

[dbrgn]

Feel free to open a request for clarification, but if there isn't any activity within - let's say - a week, I'd like to go ahead and merge this. I think that treating all like current - except for the places where considering all platforms makes sense (which is only --list, I think) - would be a very logical and easy to implement approach. If someone would specify -p all for another command, then it would simply work as it would without any -p argument (so it's essentially being ignored). However, this approach should probably be documented, so opening an issue would be the right thing to do.

If the spec would require that -p all may only be accepted in combination with --list, it would simply make argument validation harder for all clients without any benefit.

[niklasmohrin]

See tldr-pages/tldr#7528 :)

[dbrgn]

Discussion in tldr-pages/tldr#7528 and tldr-pages/tldr#7561 is still ongoing, but I'll remove this from the milestone: This definitely isn't one of the important compatibility issues we're having.

[dbrgn]

Closing for now.