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.

Sign up for GitHub

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

Jump to bottom

Adding List element validation for ValuesAre #37

Merged
merged 8 commits into from
Jun 23, 2022
Merged

Adding List element validation for ValuesAre #37

merged 8 commits into from
Jun 23, 2022

Conversation

bendbennett
Copy link
Contributor

@bendbennett bendbennett commented Jun 21, 2022 •
edited
Loading

Closes #11

@bendbennett bendbennett requested a review from a team as a code owner June 21, 2022 10:12
@bendbennett bendbennett changed the title Adding List element validation for ValuesAre (#11) Adding List element validation for ValuesAre Jun 21, 2022
detro
detro reviewed Jun 21, 2022
View reviewed changes
Copy link
Contributor

@detro detro left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I'd write here exactly the same as #36 (review), so I'm thinking: couldn't ValuesAre be turned generic?

The logic to detect if it's a List or a Set wouldn't be much more than https://github.com/hashicorp/terraform-provider-tls/pull/215/files#diff-d9583d35f5c3545409ce81ec8b21c319965974c7cb1c613b8db35c30db085c73R127

What do you recon?

@bendbennett
Copy link
Contributor Author

Following discussion with @detro we could consolidate the ValuesAre validators for List, Map and Set into a single generic validator if we don't require any type-specific validation.

bflad
bflad approved these changes Jun 22, 2022
View reviewed changes
Copy link
Contributor

@bflad bflad left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me 🚀

If there is some sort of shared implementation detail, I'd still recommend keeping a listvalidator package ValuesAre function for discoverability purposes.

@bflad bflad added this to the v0.3.0 milestone Jun 22, 2022
@bflad bflad added enhancement New feature or request type/list types.List validators labels Jun 22, 2022
bflad
bflad reviewed Jun 22, 2022
View reviewed changes
CHANGELOG.md Outdated
@@ -1,3 +1,8 @@
# 0.3.0 (unreleased)
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This file will be handled prior to release to prevent merge conflicts across PRs: https://github.com/hashicorp/terraform-plugin-framework-validators/blob/main/.github/CONTRIBUTING.md#releases

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Removed changes from CHANGELOG.md.

.changelog/11.txt Outdated
@@ -0,0 +1,3 @@
```release-note:feature
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Nit: typically these files are associated with the PR number, rather than the issue, just so its easy to handle PRs that close multiple issues

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Renamed 11.txt => 37.txt.

@bendbennett bendbennett merged commit 4af42b1 into main Jun 23, 2022
@bendbennett bendbennett deleted the bendbennett/issues-11 branch June 23, 2022 11:52
@github-actions
Copy link

I'm going to lock this pull request because it has been closed for 30 days ⏳. This helps our maintainers find and focus on the active contributions.
If you have found a problem that seems related to this change, please open a new issue and complete the issue template so we can capture all the details necessary to investigate further.

@github-actions github-actions bot locked as resolved and limited conversation to collaborators Jul 24, 2022
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@detro detro detro left review comments

@bflad bflad bflad approved these changes

Assignees
No one assigned
Labels
enhancement New feature or request type/list types.List validators
Projects
None yet
Milestone
v0.3.0
Development

Successfully merging this pull request may close these issues.

List Element Validation (ValuesAre)
3 participants
@bendbennett @detro @bflad