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 (2.0): document removal of master branch, addition of support for individual translation archives #10148

Merged
merged 8 commits into from
Sep 10, 2023

Conversation

kbdharun
Copy link
Member

@kbdharun kbdharun commented May 3, 2023

As promised in the chatroom, I plan to create a release informing that the master branch is deprecated and introduce the newer client specification after merging this PR.

Note: I have pre-referenced the tag link, which will be available once it is released. The date is tentative too, we can update it later to reflect the correct date.

Changes

  • Fix typos and grammatical errors.
  • Bump version to v2.0, minor changes in the changelog.
  • Document removal of the master branch and the added support for individual translation archives.

@github-actions github-actions bot added the documentation Issues/PRs modifying the documentation. label May 3, 2023
@kbdharun kbdharun added the clients Issues pertaining to a particular client or the clients as whole. label May 3, 2023
Copy link
Member

@pixelcmtd pixelcmtd left a comment

Choose a reason for hiding this comment

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

I don't think we need to fight over who has the worse English, but all wording/grammar/... changes seem debatable at best to me.
And what exactly is your rationale for going to 2.0? I kinda get that we might want to make a new client spec release with a few changes having accumulated, but why would it be a major? Because it's breaking for some clients? Some changes in the previous minors were possibly breaking too

@kbdharun
Copy link
Member Author

kbdharun commented May 3, 2023

And what exactly is your rationale for going to 2.0? I kinda get that we might want to make a new client spec release with a few changes having accumulated, but why would it be a major? Because it's breaking for some clients?

The main motive behind the change is that the master branch got removed, not all tldr clients are listed in the wiki, and there are many clients, extensions and plugins which still use master in order to notify them I planned on making a tagged release, I have bumped the client specification for it here, there are other additions too like one of the long options you added.

No change to date was as breaking/has a larger impact as the current one because we never removed branches before, so I think this deserves a major version bump, if other maintainers suggest a smaller bump (to v1.6) then I am open to updating it.

I don't think we need to fight over who has the worse English, but all wording/grammar/... changes seem debatable at best to me.

I want to make it clear, I am not pointing towards someone and starting a debate about whose English is better, I just made some QoL improvements which I think is right. Feel free to review it, as the client specification must be as professional as possible to the client authors, you don't need to take my word you can run the same checks through any third-party proofreading tools and you will get almost the same suggestions.

Copy link
Member

@waldyrious waldyrious left a comment

Choose a reason for hiding this comment

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

Nice work, @kbdharun! I've left some inline suggestions.

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

@SethFalco SethFalco left a comment

Choose a reason for hiding this comment

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

I much prefer the new version. It's true that some changes are subjective, but it makes the document much more accessible imo. It might've been a tad too formal before.

Now was a great time to review this while dropping the master branch anyway, and you did a good job. 👍

@kbdharun kbdharun requested review from mfrw and navarroaxel May 4, 2023 07:11
Copy link
Member

@waldyrious waldyrious left a comment

Choose a reason for hiding this comment

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

Thanks for doing this, @kbdharun — LGTM!

@acuteenvy
Copy link
Member

If we're bumping the major version, maybe we could fix #10040 while we're at it?
It's been laying around for quite a while now, this is the earliest mention I could find.

@kbdharun
Copy link
Member Author

If we're bumping the major version, maybe we could fix #10040 while we're at it? It's been laying around for quite a while now, this is the earliest mention I could find.

Anybody working on this?

@github-actions

This comment was marked as outdated.

@github-actions github-actions bot added the waiting Issues/PRs with Pending response by the author. label Jun 4, 2023
@kbdharun kbdharun added stalebot ignore and removed waiting Issues/PRs with Pending response by the author. labels Jun 8, 2023
@mrnossiom
Copy link

If we're bumping the major version, maybe we could fix #10040 while we're at it? It's been laying around for quite a while now, this is the earliest mention I could find.

I don't know if this spec is the place to define the tldr pages syntax. But if so, it should be more clear that is the intent and not just an aside note. This is why I suggest restructuring the Page structure chapter of the spec, with something like the following:

The tldr examples pages structure are written in the standard CommonMark. However, it needed the addition of a syntax to show values the user may edit to adapt the example to its needs. These parts are indicated by surrounding double parentheses, {{ for opening and }} for closing. These parentheses might need to be escaped and can be done with a backslash (\) right before the first opening tag. Clients MUST NOT break if the page format is changed within the CommonMark specification.

Examples :

  • ping {{example.com}} MUST BE rendered as "ping example.com"
  • docker inspect --format="\{{range.NetworkSettings.Networks}}\{{.IPAddress}}\{{end}}" {{container}} MUST BE rendered as "docker inspect --format='{{range.NetworkSettings.Networks}}{{.IPAddress}}{{end}}' container"

I've removed the notice at the beginning

Although this specification is about the interface that clients must provide, [...]
It seems out of place. Either, we can create another spec for the format the tldr pages MUST follow and point both this spec and the contributor guide to it, either we define the format here but assume it.

I'm not sure that we need the examples if this is clear enough, you can choose whether to include them.

In any case, I think that having a place where the format is clearly defined would be nice. I use tldr on a daily basis, and I'm very glad this exists. Thanks

@kbdharun
Copy link
Member Author

Marking the PR as a draft, later today I plan on updating it to highlight #10555

@kbdharun kbdharun marked this pull request as draft August 16, 2023 05:21
@kbdharun kbdharun marked this pull request as ready for review August 16, 2023 15:52
@kbdharun kbdharun changed the title CLIENT-SPECIFICATION: bump version to 2.0, fix typos CLIENT-SPECIFICATION (2.0): removal of master branch, addition of support for individual translation archives Aug 16, 2023
@mrnossiom
Copy link

Has changing the syntax for escaping highlight expressions been considered ?

@kbdharun
Copy link
Member Author

kbdharun commented Sep 2, 2023

Has changing the syntax for escaping highlight expressions been considered ?

Currently no, but we will look into it (and other related issues) in the future.

Offtopic:

Since this PR has been open for a while now (with new changes like translation archives), I will request final reviews in the chatroom and once the green light is given, I will create a release soon.

@kbdharun kbdharun changed the title CLIENT-SPECIFICATION (2.0): removal of master branch, addition of support for individual translation archives CLIENT-SPECIFICATION (2.0): document removal of master branch, addition of support for individual translation archives Sep 10, 2023
@kbdharun kbdharun merged commit ebd4b32 into tldr-pages:main Sep 10, 2023
3 checks passed
@acuteenvy acuteenvy removed their request for review September 10, 2023 16:09
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
Status: Done
Development

Successfully merging this pull request may close these issues.

7 participants