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

fix: range-constraint and lenght-constraint work with only one bound #548

Merged
merged 3 commits into from
May 3, 2024
Merged

fix: range-constraint and lenght-constraint work with only one bound #548

merged 3 commits into from
May 3, 2024

Conversation

TungstnBallon
Copy link
Contributor

It is now possible to only specify one bound when using lenght-constraint. However one bound minimum is still required
closes #533

I went ahead and took the liberty to also implement this fix for range-constraint.

@TungstnBallon TungstnBallon linked an issue Apr 15, 2024 that may be closed by this pull request
[BUG] <Length constraint does not work if one of the min or max values are mentioned and not both of them> #533
Closed
@TungstnBallon TungstnBallon self-assigned this Apr 15, 2024
@TungstnBallon TungstnBallon force-pushed the bug-length-constraint-does-not-work-if-one-of-the-min-or-max-values-are-mentioned-and-not-both-of-them-533 branch 3 times, most recently from 77f4594 to 5972412 Compare April 16, 2024 18:54
@TungstnBallon TungstnBallon marked this pull request as ready for review April 16, 2024 18:54
@TungstnBallon TungstnBallon force-pushed the bug-length-constraint-does-not-work-if-one-of-the-min-or-max-values-are-mentioned-and-not-both-of-them-533 branch from 5972412 to eab0e5f Compare April 16, 2024 22:17
@TungstnBallon TungstnBallon requested a review from rhazn April 16, 2024 22:25
rhazn
rhazn requested changes Apr 17, 2024
View reviewed changes
Copy link
Contributor

@rhazn rhazn left a comment

Choose a reason for hiding this comment

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

Thanks for the fix and great that you also expanded it to other constraint types, well done 🥳 . I think the approach taken is not the perfect one though, I left a comment on the execution-context towards that.

As always, I could be wrong so if there is a reason why you chose to not go the default value route let me know, otherwise I think it would be better to approach it like that.

@TungstnBallon TungstnBallon force-pushed the bug-length-constraint-does-not-work-if-one-of-the-min-or-max-values-are-mentioned-and-not-both-of-them-533 branch from eab0e5f to 0f6fcc6 Compare April 17, 2024 15:30
@TungstnBallon TungstnBallon requested a review from rhazn April 17, 2024 15:38
rhazn
rhazn requested changes Apr 18, 2024
View reviewed changes
Copy link
Contributor

@rhazn rhazn left a comment

Choose a reason for hiding this comment

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

I might be missing something but there are no functional changes in this PR, only tests and hints now? Did you merge the functional changes in a different PR?

libs/execution/test/assets/length-constraint-executor/only-lower-bound.jv Outdated
}

block TestProperty oftype TestProperty {
valuetypeAssignmentProperty: "test" oftype ConstraintType;
Copy link
Contributor

Choose a reason for hiding this comment

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

Suggested change
valuetypeAssignmentProperty: "test" oftype ConstraintType;
valuetypeAssignmentProperty: "test" oftype TestValueType;

Not sure how this model even compiles?! 🤔 Same for the other model.

@TungstnBallon TungstnBallon force-pushed the bug-length-constraint-does-not-work-if-one-of-the-min-or-max-values-are-mentioned-and-not-both-of-them-533 branch from 0f6fcc6 to dbf3670 Compare April 28, 2024 07:54
@TungstnBallon TungstnBallon force-pushed the bug-length-constraint-does-not-work-if-one-of-the-min-or-max-values-are-mentioned-and-not-both-of-them-533 branch from dbf3670 to dfb9f3c Compare April 28, 2024 07:54
@TungstnBallon
Copy link
Contributor Author

I might be missing something but there are no functional changes in this PR, only tests and hints now? Did you merge the functional changes in a different PR?

It seems to work without functional changes. I think this is because all the required properties like lowerBound or lowerBoundInclusive already have defaults that are used if the property isn't specified in the .jv file.
I followed the steps to reproduce from #533 and the error described there is gone too.

rhazn
rhazn approved these changes May 3, 2024
View reviewed changes
Copy link
Contributor

@rhazn rhazn left a comment

Choose a reason for hiding this comment

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

Sounds good, then we have hardened Jayvee for the future with more tests :). For future reference, if you'd like me to review again please always click the re-request review button though.

@TungstnBallon TungstnBallon merged commit 5e89f86 into main May 3, 2024
3 checks passed
@TungstnBallon TungstnBallon deleted the bug-length-constraint-does-not-work-if-one-of-the-min-or-max-values-are-mentioned-and-not-both-of-them-533 branch May 3, 2024 13:21
@github-actions github-actions bot locked and limited conversation to collaborators May 3, 2024
Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Reviewers

@rhazn rhazn rhazn approved these changes

Assignees

@TungstnBallon TungstnBallon

Labels
None yet
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.

[BUG] <Length constraint does not work if one of the min or max values are mentioned and not both of them>
2 participants
@TungstnBallon @rhazn