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

deprecate ExactValidArgs in favour of MatchAll(OnlyValidArgs, ...) #1646

Closed
wants to merge 1 commit into from

Conversation

umarcor
Copy link
Contributor

@umarcor umarcor commented Mar 27, 2022

This is a subset of #1643, which is a subset of #841.

@marckhouzam
Copy link
Collaborator

This is so close to #1643, I'm not sure we can't go straight to merging #1643.

Let's see which approach @jpmcb prefers.

@jpmcb
Copy link
Collaborator

jpmcb commented Mar 29, 2022

Any reason for splitting this up into multiple PRs?

Generally, I think i'd prefer to tackle in as few PRs as possible. But if it makes sense to do multiple, that's fine.

I don't have a ton of context on this, so let me familiarize myself and get back on this

@umarcor
Copy link
Contributor Author

umarcor commented Mar 29, 2022

Any reason for splitting this up into multiple PRs?

Having it all together did delay the contribution several years, because it was apparently too much to tackle. Splitting into smaller pieces is allowing to slowly get the content merged.

On the other hand, according to ab42c93, #1643 might produce an error because it modifies more than 200 lines.

I don't have a ton of context on this, so let me familiarize myself and get back on this

See #1643 (comment).

@github-actions github-actions bot added the size/M Denotes a PR that chanes 24-99 lines label Mar 30, 2022
@umarcor
Copy link
Contributor Author

umarcor commented Apr 21, 2022

@jpmcb can we please merge this M-sized PR, to move forward with the series?

@umarcor umarcor force-pushed the deprecate-exactvalidargs branch 3 times, most recently from 50536cd to ba01e0f Compare May 5, 2022 07:03
@umarcor umarcor force-pushed the deprecate-exactvalidargs branch 2 times, most recently from 3138153 to 843f4f2 Compare May 18, 2022 14:52
@umarcor
Copy link
Contributor Author

umarcor commented May 18, 2022

@jpmcb @marckhouzam @johnSchnake can we please address this before the spring is over?

@marckhouzam
Copy link
Collaborator

I vote for merging #1643 directly

@umarcor
Copy link
Contributor Author

umarcor commented May 18, 2022

#1643 is a superset, so merging any of them will solve this.

@umarcor
Copy link
Contributor Author

umarcor commented Jun 6, 2022

ping @spf13 @johnSchnake @jpmcb @marckhouzam

@marckhouzam
Copy link
Collaborator

I'm reviewing and expect to merge #1643 which is a superset of this one. So I believe we can close this one.
Please reopen @umarcor if I misunderstood.

@umarcor umarcor deleted the deprecate-exactvalidargs branch August 30, 2022 17:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
size/M Denotes a PR that chanes 24-99 lines
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants