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-SPECIFICATION: drop the special "all" platform #7561

Merged
merged 1 commit into from
Feb 18, 2022

Conversation

dbrgn
Copy link
Contributor

@dbrgn dbrgn commented Dec 19, 2021

As discussed in #7528 and here:

Drop the special all platform.

@github-actions github-actions bot added the documentation Issues/PRs modifying the documentation. label Dec 19, 2021
@dbrgn

This comment was marked as outdated.

@navarroaxel navarroaxel changed the title Client spec: Introduce --list-all as replacement for "all" platform CLIENT-SPECIFICATION: add --list-all as replacement for "all" platform Dec 20, 2021
@@ -38,7 +38,8 @@ Option | Required? | Meaning
`-v`, `--version` | Yes | Shows the current version of the client, and the version of this specification that it implements.
`-p`, `--platform` | Yes | Specifies the platform to be used to perform the action (either listing or searching) as an argument. If this option is specified, the selected platform MUST be checked first instead of the current platform as described below.
`-u`, `--update` | Conditional | Updates the offline cache of pages. MUST be implemented if cache is supported.
`-l`, `--list` | No | Lists all the pages in the current platform to the standard output. If the special platform `all` is specified a list of all pages in all platforms MUST be displayed.
`-l`, `--list` | No | Lists all the pages in the current platform to the standard output. It is RECOMMENDED that the pages are sorted alphanumerically.
`--list-all` | No | Lists all the pages in all platforms to the standard output. It is RECOMMENDED that the pages are sorted alphanumerically.
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Simply specifying sorting alphabetical ordering isn't always though. As Jeff Attwood explains, unicode can cause an issue here. We should find some standard on unicode sorting here.

The other thing we should handle here is the case of when a page exists in multiple platforms. Perhaps we should require the platform name to be specified as well here, and explicitly state that a given page name may exist in multiple categories and that's ok and they MUST NOT be deduplicated?

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, you're right. As written in my other comment above, after some thought I'd actually tend towards not adding --list-all to the standard at all. Same thing with --list, we could leave it to the implementation to choose a useful output format. (Or is there any advantage in having a common output format? Should it be parseable?)

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yeah, I think that's a good point. Clients can be for many different platforms and purposes - even if we assume a command line, there's a range of possible purposes that such a client may be implemented for. Let's drop the sorting sentence here.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I pushed a new version that simply removed the all platform and doesn't specify anything else. Implementations can still implement --list-all if they want, but if the output format isn't standardized, then a standardized command name won't bring any benefits either.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right. Let's add support for --list-all. What about specifying 1 command name per line on the standard output? e.g.

`--list-all` | No          | Lists all the pages in all platforms to the standard output, with 1 page name per line in the format `<platform>/<pagename>`

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sbrl did you take a look at this comment? I think adding --list-all to the spec isn't a good idea, there are a lot of open questions and a standardized --list-all wouldn't really give a lot of benefits while potentially limiting improvements to the output format.

My curent suggestion would be dropping the all platform and not standardizing --list-all. I renamed the PR to reflect the current state of the proposal. If you agree, this discussion can be marked as resolved.

Copy link
Member

@sbrl sbrl Feb 17, 2022

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Right, I think I see. Fair enough, in the interests of an unambiguous spec lets merge this PR as-is.

For future reference, if an argument or feature is not specified in the client specification, that doesn't mean to say that you (a client author) can't implement it :D

Sorry for taking so long about this! I've been having a rough time with my PhD and IRL stuff.

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.
@github-actions
Copy link

Hi all! This thread has not had any recent activity.
Are there any updates? Thanks!

@github-actions github-actions bot added the waiting Issues/PRs with Pending response by the author. label Jan 13, 2022
@navarroaxel navarroaxel requested a review from sbrl January 16, 2022 16:58
@dbrgn dbrgn changed the title CLIENT-SPECIFICATION: add --list-all as replacement for "all" platform CLIENT-SPECIFICATION: Drop the special "all" platform Jan 17, 2022
@github-actions
Copy link

Hi everyone.
This thread is being closed as there was no response to the previous prompt.
However, please leave a comment whenever you're ready to resume, so the thread can be reopened.
Thanks again!

@github-actions github-actions bot closed this Feb 17, 2022
@dbrgn
Copy link
Contributor Author

dbrgn commented Feb 17, 2022

github-actions: Please re-open.

@dbrgn
Copy link
Contributor Author

dbrgn commented Feb 17, 2022

@sbrl @waldyrious any opinions on this? (I updated the PR description to properly reflect the current state of the PR.)

@waldyrious
Copy link
Member

As I wasn't involved in the previous discussions, I'll let @sbrl take this one, unless there are specific aspects or open questions you'd like my thoughts on.

@waldyrious waldyrious reopened this Feb 17, 2022
@github-actions github-actions bot removed the waiting Issues/PRs with Pending response by the author. label Feb 17, 2022
@navarroaxel navarroaxel changed the title CLIENT-SPECIFICATION: Drop the special "all" platform CLIENT-SPECIFICATION: drop the special "all" platform Feb 18, 2022
@navarroaxel navarroaxel merged commit 01cb9e5 into tldr-pages:main Feb 18, 2022
@dbrgn dbrgn deleted the list-all branch February 18, 2022 10:41
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

Successfully merging this pull request may close these issues.

4 participants