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

feat: add flag type label #859

Closed
wants to merge 6 commits into from

Conversation

AllanOricil
Copy link
Contributor

@AllanOricil AllanOricil commented Nov 9, 2023

This will enhance the way (flags | args) are displayed. Developers will no longer need to read the flag's description to grasp the type|format of value they have to use with a certain flag or arg. For example, when I read filePath I immediately understand that a certain flag require a file path.

Below you can see how docker cli does it

image

obs: I added a new property called typeLabel instead of using the helpValue because I did not want to force cli developers to use it, and because the variable name better describe what it is than helpValue

BEFORE

image

AFTER

To enable this render type CLI developers have to set oclif.showFlagTypeLabel to true in the package.json

image

When typeLabel is not defined, and oclif.showFlagTypeLabel is true instead of showing =<option> it is displayed string, or string... if multiple. It feels cleaner in my opinion.

image

When combining with the theme and indent features

image

@AllanOricil AllanOricil marked this pull request as ready for review November 11, 2023 05:00
@AllanOricil AllanOricil changed the title Append (flag | arg) type label after (char and name | arg) feat: add flag type label Nov 27, 2023
@mdonnalley
Copy link
Contributor

@AllanOricil Thanks for making this PR but I don't think it's something we want to move forward with. I have two main concerns:

  1. I think it makes the help output too crowded
  2. If I understand correctly, typeLabel would need to be added to every single flag. Otherwise, it would show something like --integer string (as an example), which is confusing. So I'm not sure how it's better than the existing helpValue property

@AllanOricil
Copy link
Contributor Author

AllanOricil commented Jan 30, 2024

@mdonnalley my idea was to deprecate the other prop and eventually merge both into one. This way developers can decide what to show in their flags. In my opinion, I really think that showing directoryPath instead of <value> offers a much better dx. I also think the variable name is better than the current one.

edited: If you don't agree, or if the effort to replace the current attribute is too complicated, you can just close it. But you could see what devs prefer before closing it. Like, show some people around those pictures I posted above, and ask them which one they like the most.

@AllanOricil AllanOricil closed this Feb 9, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants