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

enable composing PositionalArgs #896

Merged
merged 4 commits into from Dec 7, 2021
Merged

enable composing PositionalArgs #896

merged 4 commits into from Dec 7, 2021

Conversation

nelz9999
Copy link
Contributor

@nelz9999 nelz9999 commented Jun 28, 2019

This may end up being helpful related to issues like #838 or #745...

@CLAassistant
Copy link

CLAassistant commented Jun 28, 2019

CLA assistant check
All committers have signed the CLA.

@umarcor
Copy link
Contributor

umarcor commented Jul 2, 2019

Hi @nelz9999! ExactValidArgs is deprecated in #841, since checks are orthogonalized: checking the number of arguments is decoupled from checking the values. Please see #841 (comment) and #841 (comment). Could you, please, have a look at #841 and propose this PR on top of it? I'll be pleased to include it, so that everything is merged together.

Apart from that, I would expect this PR to include additional tests to check the new feature. Some example would also be helpful to better show what you mean with 'composite positional arguments'.

@nelz9999
Copy link
Contributor Author

nelz9999 commented Jul 2, 2019

Hey @umarcor...

I took your suggestions of tests & docs, and to leave ExactValidArgs alone.

However, I don't think I should add on top of your #841... There's a lot of pushback and flux on that PR, which I don't think this addition needs to be a part of, since it's simply additive. I.e. this PR could get merged without any impact on #841.

@umarcor
Copy link
Contributor

umarcor commented Jul 3, 2019

I took your suggestions of tests & docs, and to leave ExactValidArgs alone.

Thanks!

However, I don't think I should add on top of your #841... There's a lot of pushback and flux on that PR, which I don't think this addition needs to be a part of, since it's simply additive. I.e. this PR could get merged without any impact on #841.

Agree. Since ExactValidArgs is not modified now, both PRs are independent. Although they cannot be automatically merged (see https://github.com/umarcor/cobra/compare/feat-matchvalid...nelz9999:nelz%2Fargs?diff=split or https://github.com/nelz9999/cobra/compare/nelz%2Fargs...umarcor:feat-matchvalid?diff=split), I believe it will not be hard to do it.

Overall, LGTM. I would just suggest to reconsider the name of the new function/feature, tho. When I read 'composing PositionalArgs' I imagined handling positional arguments composed of more than a single word. IMHO, MatchAll (as proposed by @Antolius in #745), MatchMany, MatchMultiple, CheckArgs, ArgChecks, CombinedMatch, etc. would better fit the usage: combine multiple existing checks (either built-in or custom); e.g. check ExactArgs length along with other qualities.

@nelz9999
Copy link
Contributor Author

nelz9999 commented Jul 5, 2019

@umarcor I've updated the naming.

@umarcor
Copy link
Contributor

umarcor commented Jul 6, 2019

@umarcor I've updated the naming.

I saw that and I think that this PR is ready to merge; that's why I didn't add any further comment.

Unfortunately, maintainers with write permissions in this repo seem to be quite busy lately. For example #841 is ready since almost 4 months ago, but it has not been merged yet. A similar situation happened with #817.

Hence, we need to be patient and wait until @spf13, @eparis, @bogem, or any other can allocate some time. I don't know whether @jharshman has permissions or is just a regular contributor.

Copy link
Collaborator

@jharshman jharshman left a comment

lgtm.
ping @eparis

as an aside, looks like Travis CI is failing due to some shellcheck errors.

@umarcor
Copy link
Contributor

umarcor commented Jul 8, 2019

as an aside, looks like Travis CI is failing due to some shellcheck errors.

Indeed... #889 (comment)

@umarcor
Copy link
Contributor

umarcor commented Sep 6, 2019

@nelz9999, now that CI of the master branch is fixed, would you mind rebasing? This is so that it is not shown as 'failing', which can lead maintainers to think that this is not ready.

@nelz9999
Copy link
Contributor Author

nelz9999 commented Sep 6, 2019

@umarcor Done and done!

umarcor
umarcor approved these changes Oct 2, 2019
umarcor pushed a commit to umarcor/cobra that referenced this pull request Oct 2, 2019
umarcor pushed a commit to umarcor/cobra that referenced this pull request Oct 2, 2019
umarcor pushed a commit to umarcor/cobra that referenced this pull request Oct 14, 2019
@umarcor
Copy link
Contributor

umarcor commented Dec 23, 2019

Request to be included in 1.0.0. Ref #959.

umarcor pushed a commit to umarcor/cobra that referenced this pull request Jan 4, 2020
umarcor pushed a commit to umarcor/cobra that referenced this pull request Jan 6, 2020
umarcor pushed a commit to umarcor/cobra that referenced this pull request Jan 22, 2020
umarcor pushed a commit to umarcor/cobra that referenced this pull request Jan 22, 2020
umarcor pushed a commit to umarcor/cobra that referenced this pull request Feb 3, 2020
@umarcor umarcor mentioned this pull request Feb 3, 2020
umarcor pushed a commit to umarcor/cobra that referenced this pull request Feb 20, 2020
umarcor pushed a commit to umarcor/cobra that referenced this pull request Mar 1, 2020
@github-actions
Copy link

github-actions bot commented Apr 5, 2020

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

@umarcor
Copy link
Contributor

umarcor commented Apr 5, 2020

Not stale, but ready to be merged.

umarcor pushed a commit to umarcor/cobra that referenced this pull request Apr 13, 2020
umarcor pushed a commit to umarcor/cobra that referenced this pull request May 11, 2020
@umarcor
Copy link
Contributor

umarcor commented Nov 14, 2020

Is there anything preventing this from being merged?

umarcor pushed a commit to umarcor/cobra that referenced this pull request Dec 4, 2020
umarcor pushed a commit to umarcor/cobra that referenced this pull request Feb 8, 2021
umarcor pushed a commit to umarcor/cobra that referenced this pull request Feb 21, 2021
@veleek
Copy link

veleek commented Mar 11, 2021

Ping this issue! Anything to prevent this from being merged?

umarcor pushed a commit to umarcor/cobra that referenced this pull request May 3, 2021
@katexochen
Copy link
Contributor

katexochen commented Sep 17, 2021

Will this be merged soon?

@umarcor
Copy link
Contributor

umarcor commented Sep 17, 2021

@katexochen, I wouldn't bet on that. Very unfortunately, the criteria for merging PRs in this project is arbitrary and it is frequent that ready-to-merge PRs are ignored for years.

@katexochen
Copy link
Contributor

katexochen commented Sep 17, 2021

@umarcor thanks for the answer, already thought so. And thanks to @nelz9999 for the work anyway.

umarcor pushed a commit to umarcor/cobra that referenced this pull request Nov 4, 2021
umarcor pushed a commit to umarcor/cobra that referenced this pull request Nov 4, 2021
umarcor pushed a commit to umarcor/cobra that referenced this pull request Nov 5, 2021
umarcor pushed a commit to umarcor/cobra that referenced this pull request Nov 5, 2021
umarcor pushed a commit to umarcor/cobra that referenced this pull request Nov 10, 2021
@jpmcb
Copy link
Collaborator

jpmcb commented Nov 15, 2021

Hi @nelz9999 - thanks so much for this, and thanks all for the patience. Any possibility you are around to rebase this and fix a merge conflict in the readme? If I don't hear from you in a week or so, I'll close this and hand it back over to the community.

umarcor pushed a commit to umarcor/cobra that referenced this pull request Nov 15, 2021
@nelz9999
Copy link
Contributor Author

nelz9999 commented Nov 16, 2021

@jpmcb Done!

umarcor pushed a commit to umarcor/cobra that referenced this pull request Nov 16, 2021
umarcor pushed a commit to umarcor/cobra that referenced this pull request Nov 16, 2021
jpmcb
jpmcb approved these changes Dec 7, 2021
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

7 participants