Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Client spec: Clarify behavior of special platforms #7528

Closed
niklasmohrin opened this issue Dec 11, 2021 · 6 comments
Closed

Client spec: Clarify behavior of special platforms #7528

niklasmohrin opened this issue Dec 11, 2021 · 6 comments
Labels
documentation Issues/PRs modifying the documentation.

Comments

@niklasmohrin
Copy link
Contributor

Hey everyone, I am back with another client spec topic 😅

In our effort to make tealdeer behave closer to the client specification, we came to adding support for tldr --list --platform all as described in the row for the --list flag in the Arguments table. While the behavior for --list --platform all is clear to us, we don't agree on how the platform all should behave when used in other contexts like simple page lookup. Mainly, there are two different approaches that came up so far:

  1. The platform all behaves like the current platform / as if no platform was specified at all, but for --list we keep in mind whether it was actually all or not.
  2. We accept --platform all only for --list and error if the special platform is passed for other uses.

(some arguments for either side may be found in the linked comment)

I saw that the platform all is in the spec since the beginning (#2706) and there have been other issues about the behavior when several (independent) subsystems are used together (#4290). While I don't necessarily agree with all the details in the latter, I think that the specification should specify behavior for the special platform type and its meaning in all contexts in the platform section and explain the desired behavior. In a more general sense, the Arguments table should at least list all the possible values for each flag in the row of that flag itself - it was rather unintuitive to find a mention of --platform all for me at first.

Thank you for your work, I hope we can work something out :)

@dbrgn
Copy link
Contributor

dbrgn commented Dec 11, 2021

Thanks @niklasmohrin for creating this issue! I agree, this should be clarified.

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).

@sbrl
Copy link
Member

sbrl commented Dec 11, 2021

Ah, great spot here. I wrote the original spec, so this is probably my fault as I probably didn't think of / foresee this problem at the time - so sorry about that haha 😅 This probably means that technically this is undefined behaviour in the context oft he current specification.

Indeed, we should absolutely clarify this. Both of those solutions would work - which one would you prefer? I feel like option 2 would be more semantically correct here since all doesn't make sense in the context of the page resolution algorithm?

Alternatively, we could update the spec to:

  1. Remove the special platform all altogether
  2. Add a new separate --list-all argument that lists all pages.

I feel like this option might make it cleaner and reduce confusion? Which solution would you prefer @dbrgn and @niklasmohrin?

@navarroaxel navarroaxel added the documentation Issues/PRs modifying the documentation. label Dec 12, 2021
@dbrgn
Copy link
Contributor

dbrgn commented Dec 12, 2021

Hm, if that's up for discussion, your suggestion 2 (--list-all) sounds like a much cleaner solution, because it avoids this whole ambiguity.

@niklasmohrin
Copy link
Contributor Author

I personally don't like the "all = current" approach, I would be fine with --list-all or only accepting all when --list is used. I agree, --list-all would probably be the easiest solution. That would mean that --platform all would be removed and make that a breaking change, right?

@dbrgn
Copy link
Contributor

dbrgn commented Dec 19, 2021

PR: #7561

dbrgn added a commit to dbrgn/tldr that referenced this issue Dec 28, 2021
If a client wants to list all pages, something like a `--list-all`
command is the better approach.

See discussion in tldr-pages#7528 and tldr-pages#7561.
@dbrgn
Copy link
Contributor

dbrgn commented Feb 18, 2022

#7561 was merged! So this issue can be closed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
documentation Issues/PRs modifying the documentation.
Projects
None yet
Development

No branches or pull requests

4 participants