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 v1.2: new --language flag, BCP47 to POSIX and other clarifications #3168

Merged
merged 9 commits into from Aug 23, 2019

Conversation

sbrl
Copy link
Member

@sbrl sbrl commented Jul 3, 2019

As per the discussion in #3161, this PR adds a new required --language CLI argument to the spec. Once clients have good support for it, we may consider updating the tldr page for tldr (though we should be careful to mention there that pages are returned in the system language by default).

Ping @pepa65 (tldr-bash-client, has language support already), @elecprog (elecprog/tldr, also has language support), @felixonmars (tldr-python-client), @agnivade (tldr-node-client) - please feel free to @mention anyone else who maintains a client that I've missed (I'm probably missing a bunch).

@sbrl sbrl added the clients Issues pertaining to a particular client or the clients as whole. label Jul 3, 2019
@sbrl
Copy link
Member Author

sbrl commented Jul 3, 2019

Also, should we perhaps add in a clause that says that clients should mention what version of this spec they implement in their readmes?

@@ -38,6 +42,7 @@ Argument | Required? | Meaning
`-p`, `--platform` | Yes | 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.
`-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.
`--language` | Yes | Specifies the preferred language for the page returned. Overrides the `LANG` environment variable - see the [section on translations](#translations) for more information.
Copy link
Member

Choose a reason for hiding this comment

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

Ahh .. -l is already taken. Have you thought of choosing some other single letter to have a single letter flag ? I don't know .. maybe -e, -g ?

Copy link
Member Author

Choose a reason for hiding this comment

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

Ah, good idea. Not sure what to choose that would make sense, actually.

@mebeim
Copy link
Member

mebeim commented Jul 3, 2019

As far as I am concerned, having a language flag is not necessary. Most (if not all) command line programs I've seen that support localization use environment variables to detect the language. See for example man man. This is not a compulsory nor expected feature for a client, and it would also require to cache all pages for all languages (if the client uses caching), which would not be ideal and probably not what most of the users would want (for example, I personally wouldn't really like to waste disk space holding thousands of pages translated in languages that I don't understand).

We should ask clients to automatically detect language (as we already do), with an additional encouragement to make it configurable for the user (via configuration files or command-line options), but I would not suggest it as a must have feature.

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.

As per what I just said, I would suggest the following change (see below).

I would also suggest adding the following paragraph in the Language section:

If possible, it is recommended to not only rely on the environment, but also make language configurable. Clients SHOULD therefore offer options to configure or override the language using configuration files or command line arguments (like -L, --language suggested in the arguments section above).

CLIENT-SPECIFICATION.md Outdated Show resolved Hide resolved
CLIENT-SPECIFICATION.md Outdated Show resolved Hide resolved
sbrl and others added 2 commits July 5, 2019 21:53
Co-Authored-By: Marco Bonelli <mebeim@users.noreply.github.com>
@sbrl
Copy link
Member Author

sbrl commented Jul 5, 2019

Great point, @mebeim. Ideally we want to encourage the user to use the environment variable, since that's a generally accepted standard.

Should we keep the --language after all then? 🤔

@mebeim
Copy link
Member

mebeim commented Jul 5, 2019

@sbrl yes, like I said I think we should encourage to have such flag or a config file, but not make it a must have feature.

@stale
Copy link

stale bot commented Jul 21, 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 Jul 21, 2019
@agnivade
Copy link
Member

in progress

@stale stale bot removed the waiting Issues/PRs with Pending response by the author. label Jul 21, 2019
@principis
Copy link
Contributor

  • please feel free to @mention anyone else who maintains a client that I've missed (I'm probably missing a bunch).

@sbrl tldr-sharp had language support since 1.2.0 ;)
Good to see that it'll be included in the spec. :)

@mebeim mebeim added the documentation Issues/PRs modifying the documentation. label Jul 31, 2019
@mebeim mebeim changed the title client spec v1.2: Add new --language flag Client spec v1.2: new --language flag, BCP47 to POSIX and other clarifications Jul 31, 2019
@mebeim mebeim force-pushed the edit/client-spec branch 2 times, most recently from acae6fb to 143a127 Compare July 31, 2019 18:03
@mebeim
Copy link
Member

mebeim commented Jul 31, 2019

As promised in #3183 (comment), I've integrated in this PR the changes regarding language and language tags (or, more precisely, locale tags), and I've changed the title accordingly. Since I believe that having fewer versions is better to avoid confusion, I also integrated some other changes.

I think this PR should be rebased in order to maintain the meaningfulness of the commits, but it could be also squashed: in such case please use the summary I'm adding below. Forgive the force-pushing, you can still see the commit history clearly from the relative tab.

Here's a summary of what happens in version 1.2 of the client specification:

  • Addition of the recommended -L, --language command-line option.
  • Shift from BCP-47 tags to POSIX locale names, with deprecation of previous spec versions.
  • Overall reformatting of the document: use of --- and === to define section headers and more spacing, better spacing in tables using spaces instead of tabs.
  • Rewording and reordering of the language section, also encouraging the use of configuration files for language.
  • Rewording of the term "arguments" into "options`: the values described in the table are command-line options, "argument" is a more general term.
  • Conversion of the "Other Consideration" section into "Caching" with additional clarifications: since the only thing being discussed under this section was caching.

I would suggest reading the whole document when reviewing, other than checking commits singularly, since changes are basically all around the document.

NOTE: this should be merged only after the changes discussed in #3183 are addressed. In other words, let's first apply the renaming of the folders and the change of the spec before updating the client spec as to not create confusion. As I already said in #3183, I will take care of this in a separate PR as soon as I can. Either way I'm waiting some time to see what you guys think about this before going further.

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

agnivade commented Aug 1, 2019

Just gave some minor comments. Looks good otherwise.

and the change of the spec before updating the client spec as to not create confusion.

Which other spec did you mean ?

@mebeim
Copy link
Member

mebeim commented Aug 1, 2019

@agnivade sorry, bad wording, not really a "spec", I was talking about CONTRIBUTING.md, we still talk about BCP-47 there. I will update it along with folder names in another PR.

Language specific directories must follow the pattern `pages.<language_tag>`, where `<language_tag>` is a [BCP 47](https://tools.ietf.org/html/bcp47) conforming tag in the form of `<language>[-<region>]`, where:

Fixed the typos.

@agnivade
Copy link
Member

agnivade commented Aug 1, 2019

Got it. Thanks.

@principis
Copy link
Contributor

principis commented Aug 2, 2019

Some thought I just had, what is more important, platform or language?
Not sure if this should be added to the spec.
But for example there is a page "X" and we have our default language nl_BE. LANGUAGE=fr:de:en and the platform is linux.
So what I'm doing now is checking every language for the Linux platform if it doesn't exist, look for the "common" platform. If that doesn't exist look for other platform and show a warning.

But maybe we should look for Linux and common first for each language before going to the next language?

@mebeim
Copy link
Member

mebeim commented Aug 2, 2019

@principis I thought lookup should be like this:

$ LANGUAGE="it:en" tldr some-page

pages.it/linux/some-page.md -> does not exist
pages.it/common/some-page.md -> does not exist
pages/linux/some-page.md -> does not exist
pages/common/some-page.md -> FOUND!

Display pages/common/some-page.md

BUT that's a good question... now that you make me think about it I think it would make more sense for the platform to be more important.

This is a "problem" we have because common holds pages that can also be in other platforms. If that didn't happen, giving precedence to platform or to language wouldn't change anything.

@sbrl
Copy link
Member Author

sbrl commented Aug 4, 2019

Hrm, that is indeed an issue. Looks like we've got 2 options:

  • Language-first: Choose a page from another platform and prefer pages in the specified language
  • Platform-first: Choose a page in another language in order to keep the platform the same.

I'd probably say platform-first is the better option:

pages.it/linux/some-page.md -> does not exist
pages/linux/some-page.md -> does not exist
pages.it/common/some-page.md -> does not exist
pages/common/some-page.md -> FOUND!

@mebeim
Copy link
Member

mebeim commented Aug 4, 2019

@sbrl yes, that's what I was thinking. Platform first makes more sense. We cannot do much except having common pages only if the page is the same on all systems. It's up to the client developers to choose which of the two methods makes more sense.

@principis
Copy link
Contributor

(So I wrote a long explanation but then had to leave and now it's gone...)
IMO platform is more important, I've currently implemented it like @sbrl mentioned. (With a fallback to English). But maybe this doesn't need to be in the spec? I did spend some time how to implement it correctly and consistent, so it could be added in the spec as a suggestion.

I think language-first really shouldn't be implemented (depends on how you do it). It could for example show a windows page on the linux platform because it's not in your first selected language, for example it, but in your second, en, there is a page for linux.

Platform-first doesn't have this problem.

@mebeim
Copy link
Member

mebeim commented Aug 5, 2019

@principis yes I agree that platform first is really the only one which makes sense, and I also agree that it should be in the spec at least as a "SHOULD" or "RECOMMENDED" clause. I will add it to this PR as soon as I can. Language-first could cause problems for users in situations like the one you just explained.

@stale
Copy link

stale bot commented Aug 20, 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 Aug 20, 2019
@sbrl
Copy link
Member Author

sbrl commented Aug 20, 2019

@mebeim is working on this @probot.

@stale stale bot removed the waiting Issues/PRs with Pending response by the author. label Aug 20, 2019
@mebeim
Copy link
Member

mebeim commented Aug 21, 2019

Welp, I almost forgot about this, thank you @probot 😅

I pushed the last commit applying @principis suggestions. After this, it should be pretty much ready to merge. Let me know what you guys think.

@mebeim
Copy link
Member

mebeim commented Aug 23, 2019

Ping @agnivade, looks fine to merge for me. Any comments?

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.

Since this is a big PR and I'm basically co-author of the PR I'll leave to someone else to merge. It should be fine rebased rather than squashed. If you really want to squash please keep a bullet point list of the changes in the extended commit message!

@sbrl
Copy link
Member Author

sbrl commented Aug 23, 2019

Apparently I can't approve my own PR, but this looks fine to me :D

@sbrl sbrl merged commit d9e1d59 into master Aug 23, 2019
@sbrl
Copy link
Member Author

sbrl commented Aug 23, 2019

We should have some way of @mentioning all client authors at once.

@mebeim mebeim deleted the edit/client-spec branch August 23, 2019 16:53
@principis
Copy link
Contributor

Welp, I almost forgot about this, thank you @probot sweat_smile

I pushed the last commit applying @principis suggestions. After this, it should be pretty much ready to merge. Let me know what you guys think.

(I really need to fix my notifications... Didn't see this.) it's too late but still, looks good, thanks!

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

4 participants