Skip to content

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

Merged
merged 5 commits into from
Dec 12, 2024
Merged

Add NoNullValues validators #246

merged 5 commits into from
Dec 12, 2024

Conversation

jar-b
Copy link
Member

@jar-b jar-b commented Nov 12, 2024

Adds a new NoNullValues validator to the listvalidator and setvalidator packages.

Closes #245

@jar-b jar-b requested a review from a team as a code owner November 12, 2024 18:34
@jar-b
Copy link
Member Author

jar-b commented Nov 12, 2024

I'll hold off on a CHANGELOG entry until it's determined whether this will be accepted.

Copy link
Member

@austinvalle austinvalle left a 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 like NoNullElements 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

@austinvalle
Copy link
Member

Ah, I missed your comment in the issue about the existing UniqueValues validator (as well as the ValueListsAre* validators 😄). Now I'm more on-board with your initial name.... consistency is probably more important here 🤔

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.
@jar-b
Copy link
Member Author

jar-b commented Dec 10, 2024

Thanks for taking a look at this! I added a map validator using the same NoNullValues naming convention, but I have no issues changing it to whatever you all think best describes the functionality.

austinvalle
austinvalle previously approved these changes Dec 12, 2024
Copy link
Member

@austinvalle austinvalle left a 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!

@austinvalle austinvalle added the enhancement New feature or request label Dec 12, 2024
@austinvalle austinvalle added this to the v0.16.0 milestone Dec 12, 2024
@austinvalle austinvalle merged commit 7d19faa into hashicorp:main Dec 12, 2024
6 checks passed
@jar-b jar-b deleted the no-null-values branch December 12, 2024 20:23
@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jan 12, 2025
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
enhancement New feature or request
Projects
None yet
Development

Successfully merging this pull request may close these issues.

NoNullValues validator for list and set types
3 participants