-
Notifications
You must be signed in to change notification settings - Fork 13
Add NoNullValues
validators
#246
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
Conversation
I'll hold off on a CHANGELOG entry until it's determined whether this will be accepted. |
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.
Hey there @jar-b 👋🏻 , thanks for raising the PR + issue.
This validator seems useful and I'm of the mind that moving more of the generic validation logic towards Go modules like this is a good thing 👍🏻
Couple initial thoughts:
- Did you have any other thoughts on potential names for the validator? Initially
NoNullValues
didn't register what the intent is to me. I don't have a super strong opinion on it as I don't really have a slam dunk alternative 😆. Would something likeNoNullElements
still make sense to you? Maybe @SBGoods or @bbasata have some thoughts.- That kind of manifests in some of the error messaging. i.e., the collection itself is not a null value, but one of the elements is a null value.
- Once we're on the same page with the name, would you mind implementing a similar validator for
mapvalidator
since it's also a collection?
I have some comments that are more nits about the description and error messages, but I'll save it for once we decide on the name
Ah, I missed your comment in the issue about the existing |
This validator will iterate over elements in the list, returning an error diagnostic if any null elements are detected.
This validator will iterate over elements in the set, returning an error diagnostic if any null elements are detected.
This validator will iterate over elements in the map, returning an error diagnostic if any null elements are detected.
Thanks for taking a look at this! I added a map validator using the same |
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 again for the contribution @jar-b! I have some minor nits on the documentation and we'll want a changelog for this as well.
I wanted to release this along side another PR sometime later today, so I'm just going to push the changes/changelog to your branch and merge/release, thanks!
Adds a new
NoNullValues
validator to thelistvalidator
andsetvalidator
packages.Closes #245