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

Minor fix to stringvalidator.LengthBetween() #157

Merged
merged 4 commits into from
Aug 21, 2023
Merged

Minor fix to stringvalidator.LengthBetween() #157

merged 4 commits into from
Aug 21, 2023

Conversation

chrismarget
Copy link
Contributor

I wrote some bugs by relying on LengthBetween()'s Go doc comment.

The Go doc comment now more accurately reflects the validator behavior.

An unnecessary validation (max cannot be less than zero, even without this check) was removed.

The Go doc comment now more accurately reflects the validator behavior.

An unnecessary validation (max cannot be less than zero) was removed.
@chrismarget chrismarget requested a review from a team as a code owner August 18, 2023 19:58
@chrismarget chrismarget changed the title Minor fix to LengthBetween() Minor fix to stringvalidator.LengthBetween() Aug 18, 2023
@bendbennett
Copy link
Contributor

Hi @chrismarget 👋

Thank you for the PR.

Could I ask that you add a couple of tests to verify that the "greater than or equal to" conditions are met for both minLength and maxLength. Thanks!

@chrismarget
Copy link
Contributor Author

Hi @bendbennett,

I think 00165d6 has what you're looking for.

Copy link
Contributor

@bendbennett bendbennett left a comment

Choose a reason for hiding this comment

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

LGTM 🚀

Just very minor suggestion to make test names a little more explicit.

stringvalidator/length_between_test.go Outdated Show resolved Hide resolved
stringvalidator/length_between_test.go Outdated Show resolved Hide resolved
chrismarget and others added 2 commits August 21, 2023 10:43
Suggested change from @bendbennett

Co-authored-by: Benjamin Bennett <[email protected]>
Suggested change from @bendbennett

Co-authored-by: Benjamin Bennett <[email protected]>
@bendbennett bendbennett merged commit 8e0c86c into hashicorp:main Aug 21, 2023
@bendbennett
Copy link
Contributor

Thanks for the contribution @chrismarget 👍

@bflad bflad added this to the v0.12.0 milestone Aug 21, 2023
@bflad bflad added the bug Something isn't working label Aug 21, 2023
@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 Sep 21, 2023
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants