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

Add Post terminator args support #1941

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

Conversation

malko
Copy link

@malko malko commented Mar 30, 2023

This PR addresses #1895, adding an easy way to ignore args after the arg terminator --.
It also add easy retrieving of such arguments to easily manipulate them.
It is an optin feature, without setting IgnorePostTerminatorArgs to true it should not change anything, so no breaking change.

this PR includes:

  • Adding a field IgnorePostTerminatorArgs to the Command struct
  • Adding a method PostTerminatorArgs to the Command struct
  • test we don't break normal behaviour such as args validation
  • documentation for the added feature

Hope this can be of any help to others.

- Add *Command.IgnorePostTerminatorArgs field for optin
- Add method PostTerminatorArgs to get args after arg terminator "--"
- Remove PostTerminatorArgs from args when IgnorePostTerminatorArgs=true
@CLAassistant
Copy link

CLAassistant commented Mar 30, 2023

CLA assistant check
All committers have signed the CLA.

@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.

@@ -33,78 +33,53 @@ func getCommand(args PositionalArgs, withValid bool) *Command {
}

func expectSuccess(output string, err error, t *testing.T) {
t.Helper()
Copy link
Author

Choose a reason for hiding this comment

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

Adding t.Helper() avoid IDE to jump to that function when failing

@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.

@malko
Copy link
Author

malko commented Apr 24, 2023

Is there any chance for that to be part of next release ?

@adrian-gierakowski
Copy link

I would really like to use this. Any chance for getting it accepted and released?

@malko
Copy link
Author

malko commented Sep 5, 2023

@adrian-gierakowski waiting for this since march 2023, the PR is now conflicting 😢 .

I'm ok to make the extra work to get rid of conflict if this is not more that the user_guide.md to change, but the more maintainers wait to review this the more work risks to arise.
What i'm trying to say, is that I won't make any more work on this until an official maintainer ask for it. This PR took me a lot of time, and for now even if I understand that maintainers have a lot to review and do to maintain the project, it looks to me like I've wasted my times trying to propose a PR.

Hope future will prove me wrong.

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.

None yet

3 participants