Skip to content
New issue

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

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

Already on GitHub? # to your account

windows/curl,windows/wget: fix tldr client param inconsistencies #8297

Merged

Conversation

reinhart1010
Copy link
Collaborator

  • The page(s) are in the correct platform directories: common, linux, osx, windows, sunos, android, etc.
  • The page(s) have at most 8 examples.
  • The page description(s) have links to documentation or a homepage.
  • The page(s) follow the content guidelines.
  • The PR title conforms to the recommended templates.
  • Version of the command being documented (if known):

Due to tldr-pages/tldr-node-client#336 impacting some Windows users who are using the Node.js version of tldr client, I decided to add the non-standard --os version to the documentation as a workaround.

Hopefully tldr-pages/tldr-node-client#337 can be merged to fix this issue.

@github-actions github-actions bot added the page edit Changes to an existing page(s). label Aug 4, 2022
Copy link
Collaborator

@marchersimon marchersimon left a comment

Choose a reason for hiding this comment

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

The client specification doesn't require clients to have long options. When it comes to everything tldr-related, we should always stick to the client specification. This way we can be sure, that every client supports the -p flag.

Also, which client uses --os?

@reinhart1010
Copy link
Collaborator Author

Only the Node.js client uses --os - see tldr-pages/tldr-node-client#336

@marchersimon Maybe I should stick back using -p and -o?

@marchersimon
Copy link
Collaborator

marchersimon commented Aug 8, 2022

Yes, we should definitively use the short options, we already had to reject some PRs about exactly this in the past.

I'm not so sure regarding the -o flag. We don't even mention it in the main tldr page, so why should we add it here? (Also, I actually have a PR open to fix this issue, but it seems a little stuck → tldr-pages/tldr-node-client#337)

@reinhart1010
Copy link
Collaborator Author

I'm not so sure regarding the -o flag. We don't even mention it in the main tldr page, so why should we add it here?

Our client spec doesn't require clients to output the version (i.e. Node.js, C++, Python instead of 1.0.1) this tldr client came from, on their -v and -h, making it difficult for existing Windows users to check whether they are using the "correct" (spec-compliant version) of tldr client.

Sure, Windows users can check the directory where tldr was run by using where tldr or Get-Command tldr (equivalent to which tldr in Windows Command Prompt and PowerShell).

tldr-pages/tldr-node-client#336 already breaks some Windows users who installed tldr via Node.js as they were unable to view the original curl/wget documentation using tldr curl -p common. So I have to mention a workaround instead until tldr-pages/tldr-node-client#337 is merged (and wait for months until everyone gets the latest Node.js version of tldr).

@marchersimon
Copy link
Collaborator

@CleanMachine1 @sbrl @navarroaxel any thoughts on this? If we add the -o option here, should we maybe also add it to the tldr page itself?

@navarroaxel
Copy link
Collaborator

I think we should stick to the client specification, otherwise someone can ask to add another flag used in another tldr client.
Also, @marchersimon why you don't finish the node client PR? 😄

@marchersimon
Copy link
Collaborator

Also, @marchersimon why you don't finish the node client PR? 😄

That's because I hardly know any javascript. I just replaced the occurences of --os with --platform. This would remove the old --os` flag, which introduces a breaking change. And now nobody really wants to finish it. 😄

@sbrl
Copy link
Member

sbrl commented Aug 9, 2022

Ummm the client spec does require you to output the version of the spec you implement: https://github.com/tldr-pages/tldr/blob/main/CLIENT-SPECIFICATION.md?plain=1#L38

-v, --version | Yes | Shows the current version of the client, and the version of this specification that it implements.

....and then slightly earlier:

A number of command-line options MUST be supported (unless otherwise specified) if a CLI is implemented:

...indeed, the Node.js client using --os predates the client spec, and is a known issue nobody has gotten around to fixing yet.

Ideally, the Node.js client should be updated rather than this page being updated to workaround the actual issue.

@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 Aug 25, 2022
@EmilyGraceSeville7cf EmilyGraceSeville7cf added stalebot ignore and removed waiting Issues/PRs with Pending response by the author. labels Aug 25, 2022
@reinhart1010
Copy link
Collaborator Author

Hello, is there any updates about this?

Copy link
Member

@kbdharun kbdharun left a comment

Choose a reason for hiding this comment

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

LGTM, Thanks for your contribution.

Requesting review @navarroaxel

Copy link
Collaborator

@marchersimon marchersimon left a comment

Choose a reason for hiding this comment

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

This has been waiting long enough.
Thanks @reinhart1010

@marchersimon marchersimon changed the title windows/curl, windows/wget: fix tldr client param inconsistencies windows/curl,windows/wget: fix tldr client param inconsistencies Oct 16, 2022
@marchersimon marchersimon merged commit e9d75c2 into tldr-pages:main Oct 16, 2022
@reinhart1010 reinhart1010 deleted the fix-tldr-param-inconsistency branch April 30, 2023 08:29
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
page edit Changes to an existing page(s).
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants