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(command): adding args to use line #1210

Open
wants to merge 2 commits into
base: main
Choose a base branch
from
Open

Conversation

danmx
Copy link

@danmx danmx commented Aug 27, 2020

Adding args to use line.

Old:

❯ ./test
Error: accepts 1 arg(s), received 0
Usage:
  test [flags]

Flags:
  -h, --help     help for test
  -t, --toggle   Help message for toggle

New:

❯ ./test
Error: accepts 1 arg(s), received 0
Usage:
test [flags] [args]

Flags:
-h, --help     help for test
-t, --toggle   Help message for toggle

Diff:

3c3
<   test [flags]
---
>   test [flags] [args]

@CLAassistant
Copy link

CLAassistant commented Aug 27, 2020

CLA assistant check
All committers have signed the CLA.

@github-actions
Copy link

This PR is being marked as stale due to a long period of inactivity

@danmx
Copy link
Author

danmx commented Oct 27, 2020

@jpmcb could you take a look?

@umarcor
Copy link
Contributor

umarcor commented Oct 27, 2020

Ref #842

@jpmcb
Copy link
Collaborator

jpmcb commented Dec 29, 2020

Hi @danmx thanks for this PR - would you be able to add a test for this? Maybe in command_test.go? A passing test will help this get reviewed and merged in

@github-actions github-actions bot added the size/S Denotes a PR that chanes 10-24 lines label Aug 18, 2022
@danmx danmx force-pushed the useline_args branch 2 times, most recently from fc97c0c to e481b1f Compare September 24, 2023 16:07
@danmx
Copy link
Author

danmx commented Sep 24, 2023

@jpmcb took me some time but I've added tests for UseLine.

@danmx danmx force-pushed the useline_args branch 5 times, most recently from 0179759 to 64cff20 Compare September 24, 2023 16:29
@github-actions
Copy link

This PR exceeds the recommended size of 200 lines. Please make sure you are NOT addressing multiple issues with one PR. Note this PR might be rejected due to its size.

@danmx
Copy link
Author

danmx commented Oct 1, 2023

@marckhouzam I see that you were active recently. Could you take a look at this PR?

I addressed the previous comment about adding tests.
I had to reshuffle Command struct optimal memory usage (fixing violations of maligned linter) because I added another bool field.

danmx and others added 2 commits October 17, 2023 13:02
Signed-off-by: danmx <daniel.iziourov@elastic.co>
Signed-off-by: danmx <daniel@iziourov.info>
@github-actions
Copy link

This PR exceeds the recommended size of 200 lines. Please make sure you are NOT addressing multiple issues with one PR. Note this PR might be rejected due to its size.

Copy link

This PR exceeds the recommended size of 200 lines. Please make sure you are NOT addressing multiple issues with one PR. Note this PR might be rejected due to its size.

@marckhouzam
Copy link
Collaborator

@danmx could you show the change in output that this provides in the description of the PR so I can know what to expect?

@danmx
Copy link
Author

danmx commented Nov 23, 2024

@marckhouzam the change is:

Old:

❯ ./test
Error: accepts 1 arg(s), received 0
Usage:
  test [flags]

Flags:
  -h, --help     help for test
  -t, --toggle   Help message for toggle

New:

❯ ./test
Error: accepts 1 arg(s), received 0
Usage:
test [flags] [args]

Flags:
-h, --help     help for test
-t, --toggle   Help message for toggle

Diff:

3c3
<   test [flags]
---
>   test [flags] [args]

Copy link

This PR exceeds the recommended size of 200 lines. Please make sure you are NOT addressing multiple issues with one PR. Note this PR might be rejected due to its size.

@marckhouzam
Copy link
Collaborator

Could you not reorder the fields of the command structure? Although your order may be better it makes the review harder. It’s hard for me to find time for reviews so if you keep the PR as small as possible it’ll increase your chances of getting it merged.

@marckhouzam
Copy link
Collaborator

test [flags] [args]

I don’t think we can do this. The command.Use field is defined by the program and many programs specify the arguments the command expects in their Use field. In such cases the new [args] would be repeating.

For example https://github.com/helm/helm/blob/a3c903e4c62839d24389a1e0cdf550a8d759f054/cmd/helm/pull.go#L49

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
size/S Denotes a PR that chanes 10-24 lines
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants