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.md: Write initial client specification #2706

Merged
merged 35 commits into from Mar 12, 2019
Merged

Conversation

sbrl
Copy link
Member

@sbrl sbrl commented Jan 12, 2019

It's finally happened! It's taken me all afternoon, but I've taken my previous notes & all the feedback, and folded it into a full specification, which I'm officially proposing with this PR.

Closes #1065 (finally!)

@sbrl sbrl added documentation Issues/PRs modifying the documentation. clients Issues pertaining to a particular client or the clients as whole. labels Jan 12, 2019
CLIENT_SPECIFICATION.md Outdated Show resolved Hide resolved
CLIENT_SPECIFICATION.md Outdated Show resolved Hide resolved
Copy link
Contributor

@jedahan jedahan left a comment

Choose a reason for hiding this comment

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

This looks really good, I just have one minor and one major question to help disambiguate things.

CLIENT_SPECIFICATION.md Outdated Show resolved Hide resolved
Copy link
Member

@owenvoke owenvoke left a comment

Choose a reason for hiding this comment

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

Just one minor question, looks good otherwise.

Also, there seems to be an inconsistency between paragraph sections. Should it be 1 or 2 blank lines? 🙂

CLIENT_SPECIFICATION.md Outdated Show resolved Hide resolved
@vladimyr
Copy link
Contributor

👍for your effort and produced result. Looks really nice.

However I spotted one wild inconsistency. In the same time supporting --update (cache) cli flag is mandated but later on whole cache functionality is declared as optional? 🙄
Am I missing something? 🤔

@sbrl
Copy link
Member Author

sbrl commented Jan 14, 2019

Regarding paragraph sections, it's 2 if it's 2nd level heading, and 1 if it's 3rd level or lower.

@vladimyr: Great point! I'll change that now.

Copy link
Member

@agnivade agnivade left a comment

Choose a reason for hiding this comment

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

Commented inline.

CLIENT_SPECIFICATION.md Outdated Show resolved Hide resolved
------------------------|--------------------
`--update` | Updates the offline cache of pages. MUST NOT be implemented if cache is not supported.
`--version`, `-v` | Shows the current version of the client, and the version of this specification that it implements.
`--list-all`, `-a` | Lists all the pages in the current platform to the standard output. One page name per line. Additional decoration MAY be printed to the standard error.
Copy link
Contributor

Choose a reason for hiding this comment

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

how about --list, -l? This way it is similar to ls -l (one per line), and shorter to type.

Copy link
Member Author

Choose a reason for hiding this comment

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

Hrm, you raise a good point. What do you think, @agnivade / @pxgamer / @waldyrious?

Copy link
Member

@owenvoke owenvoke Jan 15, 2019

Choose a reason for hiding this comment

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

I think this was because the node client uses --list-all 🤔 I think --list is used to show pages for the current platform.

See documentation here.

Copy link
Member Author

Choose a reason for hiding this comment

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

I think --list-all is rather confusing, actually. It implies that all pages are listed, but in reality it's only the pages in the current platform & common that are listed.

Perhaps we can disambiguate this somehow?

Copy link
Contributor

Choose a reason for hiding this comment

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

Personally I don't see any value in listing every possible page, crossplatform (which I would assume --list-all does as @jedahan pointed out) because that means we should print out paths to disambiguate pages covering same tool on different platforms. That essentially puts us in business of re-implementing ls -R. If user really wants that they should be able to get cache location somehow and do that on they own.
So my vote goes to -l, --list to get list of pages for assumed/specified platform.

OTOH discussion about list formatting is (sorry but I have to say it) aiming at completely wrong target. It simply does NOT matter at all how it will look and there is good background reason for that. Take our beloved ls for example. Default output of ls is columned or is it?! Pipe that through cat/less and you'll get one-per-line list. I'm too lazy to dig out proper POSIX/TAOUP/whatever spec/recommendation so rule of thumb instead:

Output of first class citizen *NIX CLI tool:

  1. MAY be aesthetically pleasing with all kind of wonders you decide to throw in
    if STDOUT is NOT TTY
  2. MUST be formatted as one-per-line list (to enable easy manipulation using *NIX tools)
    if STDOUT is a TTY

Can we just replace blah blah CLI tool with tldr client and put it inside spec? 😉

Copy link
Contributor

Choose a reason for hiding this comment

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

I still think if people want to do something that technical, they could either just turn to the cache directory and get anything they need there, or, they should use the -t/--tty flag or -n/--noformat or some such. Because if people pipe their output into less, they probably don't want to automatically get no columns or colors. If you use a flag, the user can decide, if you don't, some users are going to request it as a feature.

Copy link
Contributor

@vladimyr vladimyr Jan 17, 2019

Choose a reason for hiding this comment

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

@pepa65

If you use a flag, the user can decide, if you don't, some users are going to request it as a feature.

And that's like only thing I agree with. 😞

I still think if people want to do something that technical,

Calling list manipulation with standard cli tools technical is bit far fetched if you ask me; after all isn't one of selling points of tldr pages is to get people to that level more easily? 🙃
What so technical in grepping large lists? 🤔

or, they should use the -t/--tty

I'm very curious to see example of tool implementing given option. I never seen something like that and I highly doubt I ever will; having in mind it is not considered standard:

Because if people pipe their output into less, they probably don't want to automatically get no columns or colors.

Sorry but that's just your opinion while common practice teaches us exact opposite. I may or may not agree with you but you are actually arguing with facts:

So, yeah you are right, people like ansi colors but like and expect by default are two different things. By default colors are off. Which brings me to the next thing you said:

or -n/--noformat or some such.

Again, you are going against common practice established by *NIX cli tools. Formatting is off by default if output is tty. If you really want to retain it you need to explicitly request tool generating output to do so. And it makes perfect sense why you need extra step for colors/formatting to be outputted/preserved: not every tool you may use to process output understands and/or is able to strip out ansi escapes! If user is positive that his tool of choice (e.g. less) is capable of doing so, ok, you have choice to do --color=always, --color, --pretty, you name it...

In conclusion - to actually address your concern we should probably support --pretty option that would force colors and general formatting being preserved when output is not tty. I'm ok with that and FWIW you have my 👍 there but in form of recommendation, such thing shouldn't be a MUST but MAY instead:

  • stdout is TTY - Generate formatted output (optional)
  • stdout is NOT TTY and --pretty - Preserve formatting (optional)
  • stdout is NOT TTY - Generate \n separated list (mandatory)

That being said, please try to align your proposals with established CLI tools design practices and user expectations. TBH I understand your motivation and would sometimes give everything for a chance to rewrite history of our discipline but that's simply not possible. tldr clients are not first tools of such class and less exotic they are more successful and useful they'll become!

Copy link
Contributor

@vladimyr vladimyr Jan 17, 2019

Choose a reason for hiding this comment

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

@pepa65

Don't you feel you are being unnecessarily redundant with tldr --list --platform macos? I just do tldr --list macos and it works. There is no need there for a --platform marker.

Yes and no 🤔

Sure it is verbose, I personally don't like it too but...
OTOH lets compare this with what man does (I like to consider tldr client as man for humans same way as httpie is curlfor humans; correct me if my understanding is wrong :bowtie:).

In order to get all man pages you need to run:

$ man -k .

where . is regex meaning everything. If you want to narrow that down using sections:

$ man -k . -s1

lists only manpages inside 1st section.

If you now swap -k . with -l and -s1 with -p <platform> you'll get original form I proposed:

$ tldr -l -p macos
$ # or if you like it more
$ tldr --list --platform macos

You could OFC argue that manpage listing I just described has mandatory search param while tldr listing does not thus putting us in better position by allowing what you suggested. Yeah, that's true but might be also an indication that we are doing something wrong ⚠️
Maybe right way to do the listing is to copy good ol' man? 🙃

@jedahan
Could not support you more in that consistency effort 💯

@sbrl
I wanted to do some wildcard like thingie and ended up doing lousy job 🤦‍♂️

I know it isn't constructive at all but this whole thing with what should list (almost) everything print is painfully convoluted and more we evolve it less value I see in it. Can someone ELI5 in what particular case user wants to print every or almost every possible command out apart from pure curiosity and explorer's spirit?
Aren't the real life usecases:
(step zero: you know what you want to do)

  1. You know which tool can help you but you don't know how to use it so you do:
$ tldr <tool>
  1. You do NOT know which tool you should use so you do:
$ tldr --search <query>

Which isn't even mentioned in the spec itself! ⚠️

In both cases you might be searching answers for a platform you are currently not working on so you'll add --platform <target_platform> too.

I mean this is exactly what man supports also:

$ man <tool>
$ man -k <query> # analogous to `--search`

while listing all pages is available by means of empty query but it certainly isn't something that is endorsed by having dedicated option like --list-(almost/maybe/platform/common)-all 🙃

Copy link
Member

Choose a reason for hiding this comment

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

Alright then, let's go with default being current platform and having a reserved platform all.

Copy link
Member Author

Choose a reason for hiding this comment

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

Great discussion! I'll update the spec to reflect this.

Met me know if I miss anything :P

Copy link
Contributor

@pepa65 pepa65 left a comment

Choose a reason for hiding this comment

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

I think it is useful to specify the 'special' platform identifiers all common and current, to refer to respectively all pages, the pages under the common directory and the pages from both the current and the common directories. But it could be a MAY: tldr -l all, tldr -l current, tldr --list common.

As a note, in the (former?) reference implementation of tldr, --list-all is listing both common and the current platform. So this spec changes that into only the current platform?? I think then the flag's name becomes very misleading!

Argument | Meaning
------------------------|--------------------
`--update` | Updates the offline cache of pages. MUST be implemented if cache is supported.
`--version`, `-v` | Shows the current version of the client, and the version of this specification that it implements.
Copy link
Contributor

Choose a reason for hiding this comment

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

I believe we need -h, --help here too?
BTW options are typically written in reverse order: -v, --version :bowtie:

@jedahan
Copy link
Contributor

jedahan commented Jan 17, 2019

Also note as a general philosophy I think we should strive to only specify the most important features as minimally as possible to allow clients to experiment with ux/ui. The specification gives us a decent starting checklist if a client should be included in the readme/docs/wiki/whatever.

I.e. if there is any contention in how the interface acts, feel free to let different clients behave differently. For example, if people have opinions about the formatting of list that differ, allow the clients to differ in how they display lists. We can always add restrictions or suggestions to specification, but its much harder to take them away.

As a user, the only things I care about are:

  • I can find a page with tldr <command>
  • I can get the help for a given tldr client with tldr (or ideally tldr tldr)

Every other feature/idea is extra - certainly useful, and will make me choose one client over another. But if the specification defines the implementation for all clients, then I think its actually worse for the user in choosing a client. If they all work the same which should I use?

It would be a good smoke test of our specification - can the specification of tldr clients be sufficiently covered by tldr tldr?

@sbrl
Copy link
Member Author

sbrl commented Jan 18, 2019

Currently, we have just this in tldr tldr:

#tldr

- Simplified man pages.

> Get typical usages of a command (hint: this is how you got here!):
 tldr command

...because every client is different enough that tldr command is the only commonality between them all :P

CLIENT_SPECIFICATION.md Outdated Show resolved Hide resolved
CLIENT_SPECIFICATION.md Outdated Show resolved Hide resolved
@mebeim
Copy link
Member

mebeim commented Jan 22, 2019

I would recommend only making the short command line options required, with optional support to the long options. That is, for example: require -l, and optionally allow --list. I also only see --update, but not -u, which IMHO would make more sense to have by default

The reason I say this is because parsing long options is something that is not well standardized in low-level (mostly compiled) languages, and would force whoever wants to write a client to go through the seemingly unneeded hassle of creating a cross-platform long option parser or detect and use an existing one.


Stated the above, I would suggest the following changes.

First of all, rephrase the "Arguments" section that currently says:

A number of arguments MUST be supported [...]
(table of options)

to the following:

A set of options needs to be supported according to the following table:
(table of options)

Secondly, rearrange the table of options like this:

Option Meaning Notes
-u Update the offline cache of pages. MUST be supported if the program implements caching. MAY be supported otherwise.
-v Show the current version of the client, and the version of this specification that is implemented. MUST be supported.
-l [platform] Lists all the pages in the specified platform to standard output, defaulting to the current platform if none is specified. If the special platform all is specified, all pages in all platforms are listed instead. MUST be supported.
-p <platform> Specifies the platform that should be used for resolving page names. If this option is specified, the named platform MUST be checked first instead of the current platform as described below. MUST be supported.
-r Forces the output to contain all additional decorations, even if the standard output is not a TTY. MAY be supported.
--update Long alias for -u. MAY be supported.
--version Long alias for -v. MAY be supported.
--list [platform] Long alias for -l. MAY be supported.
--platform <platform> Long alias for -p. MAY be supported.
--rich Long alias for -r. MAY be supported.

PS:
I would also love to see an optional -k option mentioned (analogous to the one man has), and probably even having -h would be a good idea. As @vladimyr already suggested.

@mebeim
Copy link
Member

mebeim commented Jan 22, 2019

Also, posting this as a separate comment since the previous is pretty long: I think that having two options that take a platform as a parameter is overkill. I think only having the -p option to take a platform as parameter and then use it as a modifier would be way cooler. Something like this:

Command Behavior
tldr -l List all pages for the default (current) platform.
tldr -l -p linux List all pages for the linux platform.
tldr -lp all List all pages for all the platforms.
tldr -p linux command Search a command for the linux platform.

By the way, I don't think the list option is that useful to be a required option. I personally never had the need to list all pages in man, tldr or similar tools. It could just be me, but I'd honestly just make it an optional feature.

@mebeim
Copy link
Member

mebeim commented Feb 16, 2019

@sbrl hmm, what about:

Specifies the platform to be used to perform the action (either listing or searching). If this option is specified, the selected platform MUST be checked first instead of the current platform as described below.

Right now I can't think of a nicer way, but I'm sure there is. Maybe someone else could improve it.

By the way, I would suggest to write required options first, followed by not required options. Mixing them doesn't feel really clean.

CLIENT-SPECIFICATION.md Outdated Show resolved Hide resolved
CLIENT-SPECIFICATION.md Outdated Show resolved Hide resolved
@sbrl
Copy link
Member Author

sbrl commented Feb 19, 2019

Thanks for the suggestions! I've applied them.

@stale
Copy link

stale bot commented Mar 6, 2019

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

@stale stale bot added the waiting Issues/PRs with Pending response by the author. label Mar 6, 2019
@mebeim
Copy link
Member

mebeim commented Mar 7, 2019

I think this looks good enough to be introduced as first ever version of the client specification. Can't really keep track of all that has been said above, but looks fine, am I right? Major points and issues seem to have been addresses by @sbrl, so I'd say let's just remove the draft notes in the document and change pages/common/tldr.md to only show the options that MUST be supported and then it's ready to merge.

What do you guys think?

@stale stale bot removed the waiting Issues/PRs with Pending response by the author. label Mar 7, 2019
@agnivade
Copy link
Member

agnivade commented Mar 7, 2019

Sounds good. This has been sitting for too long. Let's give it one more day to see if anyone has any more comments, then it can be merged.

Copy link
Member

@mebeim mebeim left a comment

Choose a reason for hiding this comment

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

Good to merge to me after the following minor final details are taken care of.

CLIENT-SPECIFICATION.md Outdated Show resolved Hide resolved
pages/common/tldr.md Outdated Show resolved Hide resolved
pages/common/tldr.md Outdated Show resolved Hide resolved
mebeim and others added 3 commits March 12, 2019 11:47
Co-Authored-By: sbrl <sbrl@starbeamrainbowlabs.com>
Co-Authored-By: sbrl <sbrl@starbeamrainbowlabs.com>
@sbrl
Copy link
Member Author

sbrl commented Mar 12, 2019

Thanks everyone! I would have taken a look at this sooner, but I've been busy with work from University.

Merging now, since I've handled @mebeim's suggestions 😺

@sbrl sbrl merged commit 31d784b into master Mar 12, 2019
@sbrl
Copy link
Member Author

sbrl commented Mar 12, 2019

Oh yeah, we should add a link to it to the README too, and possibly look at which clients adhere tot he spec.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
clients Issues pertaining to a particular client or the clients as whole. documentation Issues/PRs modifying the documentation.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

7 participants