-
Notifications
You must be signed in to change notification settings - Fork 668
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
separate args validation for better reuse #899
separate args validation for better reuse #899
Conversation
a7i
commented
Aug 5, 2022
- Separate validations for reuse
- Aggregate all errors as opposed to stopping on first validation error
/cc @damemi @ingvagabund |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Thanks for doing this work @a7i, but this change wasn't really my intent with the comment in #861 (comment). The point I'm trying to get at more is that our default plugins shouldn't be any different from theoretical third-party plugins. So, not dependent on any internal functions like this.
In my other comment, I meant more that Namespace and Label arg validation should be public, common functions that we export as part of the framework (to be used by any plugins). Does that make sense?
So basically move the new functions to a new file and make them public. Do you have a place in mind that will fit this? |
@a7i I think for now they can stay where they are, I just wanted to make a note of my intent for these eventually. Breaking them into individual functions works toward that goal so this PR looks fine to me. Sorry for the confusion |
/approve |
[APPROVALNOTIFIER] This PR is APPROVED This pull-request has been approved by: damemi The full list of commands accepted by this bot can be found here. The pull request process is described here
Needs approval from an approver in each of these files:
Approvers can indicate their approval by writing |