Skip to content

Implement option length selection with options and environment variables #259

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

Merged
merged 26 commits into from
Mar 25, 2025

Conversation

Managor
Copy link
Contributor

@Managor Managor commented Dec 13, 2024

Initial ideas on how to implement the shortform/longform print modes.

@Managor Managor marked this pull request as draft December 13, 2024 16:27
@Managor
Copy link
Contributor Author

Managor commented Dec 13, 2024

Related issue tldr-pages/tldr#13556

@sbrl
Copy link
Member

sbrl commented Dec 13, 2024

Seems interesting. Does this always hold up with all instances of {{a|b}} where {{-a|--b}}?

What would you envision the CLI to look like for tldr? Could you disable the short/longform parsing support and display everything at all? What would the default be?

@acuteenvy
Copy link
Member

What about tools that don't follow this convention for short and long options (e.g. tldr-pages/tldr#13556 (comment))?
I don't think there's a way to fully support option selection without introducing special syntax as discussed in tldr-pages/tldr#5092.

@Managor
Copy link
Contributor Author

Managor commented Dec 13, 2024

If a tool doesn't follow this convention then it won't be affected by this. Simple as that.

@acuteenvy
Copy link
Member

This regular expression won't work if there are multiple short options in a single placeholder.
Example:
https://github.com/tldr-pages/tldr/blob/3823ba7fc64e2c368e87950f1aaf407d88f6957f/pages/common/rsync.md#L17

`rsync {{-zvhP|--compress --verbose --human-readable --partial --progress}} {{path/to/source}} {{path/to/destination}}`

@acuteenvy acuteenvy changed the title Initial work on option lenght selection Initial work on option length selection Dec 14, 2024
@Managor
Copy link
Contributor Author

Managor commented Dec 14, 2024

That is by design. Like I said in the original issue, the initial implementation should be as strict as possible.

EDIT: However. That example is pretty nice for condensing the shortform options. Maybe I should allow it to be picked up.

@Managor
Copy link
Contributor Author

Managor commented Dec 15, 2024

Alright, I'm looking for feedback again. Is this the proper way to implement options? Also how can I implement TLDR_SHORTFORM and TLDR_LONGFORM env variables?

@Managor
Copy link
Contributor Author

Managor commented Dec 15, 2024

Nvm figured it out. Now I'm just looking for feedback.

@Managor Managor marked this pull request as ready for review December 20, 2024 02:58
@Managor Managor changed the title Initial work on option length selection Implement option length selection with options and environment variables Dec 20, 2024
@Managor
Copy link
Contributor Author

Managor commented Jan 28, 2025

I adjusted the client specs in tldr-pages/tldr#15253 and adjusted the code to match it. I'm looking for feedback.

@Managor
Copy link
Contributor Author

Managor commented Jan 28, 2025

Demonstrative images
Input:
image

tldr rsync
image

tldr -S rsync
image

tldr -V rsync
image

tldr -VS rsync
image

@Managor
Copy link
Contributor Author

Managor commented Jan 28, 2025

grep -rE "{{\[.*\|.*\]}}" shows no matches. Accidental usages of this pattern should not be found.

@Managor
Copy link
Contributor Author

Managor commented Jan 28, 2025

@kbdharun You seem to be fluent in manpage language. Would you be willing to make an addition to the tldr manpage after this goes through?

@kbdharun
Copy link
Member

@kbdharun You seem to be fluent in manpage language. Would you be willing to make an addition to the tldr manpage after this goes through?

Sure

@Managor
Copy link
Contributor Author

Managor commented Jan 29, 2025

Updated tldr -VS rsync
image

@Managor
Copy link
Contributor Author

Managor commented Mar 7, 2025

I implemented the final command line options seen in the release. I'm looking for feedback again.

@Managor
Copy link
Contributor Author

Managor commented Mar 24, 2025

@sebastiaanspeck I would like to write tests for option placeholders. Do you have any tips the process and where to start?
I would like to test with only environment variables, only options, mixed environment variables and options and finally garbled environment variable contents.

@sebastiaanspeck
Copy link
Member

@sebastiaanspeck I would like to write tests for option placeholders. Do you have any tips the process and where to start?

I would like to test with only environment variables, only options, mixed environment variables and options and finally garbled environment variable contents.

This Python script has some tests https://github.com/tldr-pages/tldr/blob/main/scripts/_common.py

@Managor
Copy link
Contributor Author

Managor commented Mar 25, 2025

Lets merge this. I tested manually that this works. I might write tests in a separate PR. Requesting reviews.

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 the additions. Tested the changes locally and it works as expected. As promised before, will update the man page once the PR gets merged.

Adding screenshots for future reference:

Default Behaviour With short option flag passed
tldr python client default behaviour showing long options with short options

@sebastiaanspeck sebastiaanspeck merged commit d078665 into tldr-pages:main Mar 25, 2025
8 checks passed
@Managor Managor deleted the option branch March 25, 2025 14:06
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants