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

Update validator to support floats in min/max and disallow string types #230

Merged
merged 27 commits into from
Jan 18, 2024
Merged

Update validator to support floats in min/max and disallow string types #230

merged 27 commits into from
Jan 18, 2024

Conversation

VirajP1002
Copy link
Contributor

@VirajP1002 VirajP1002 commented Dec 11, 2023 •
edited
Loading

What is the context of this PR?

Provide additional validation to ensure minimum/maximum values set in JSON are not of string types and change JSON schema definitions to accept floats.

How to review

  • Ensure changes to the schemas are correct and the Python method is suitable

Checklist

  • eq-translations updated to support any new schema keys which need translation

@VirajP1002
Copy link
Contributor Author

Update validator to support float minimum/maximum

@VirajP1002 VirajP1002 changed the title Support float max or min Update validator to support floats in min/max and disallow string types Dec 11, 2023
@MebinAbraham MebinAbraham added the Enhancement New feature or request label Dec 14, 2023
katie-gardner
katie-gardner previously requested changes Dec 14, 2023
View reviewed changes
Copy link
Contributor

@katie-gardner katie-gardner left a comment

Choose a reason for hiding this comment

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

This will need something a little different to handle value sources that reference another answer, you'll likely need to look that up in one of the existing questionnaire schema maps to find the type of the source

app/validators/answers/number_answer_validator.py Outdated Show resolved Hide resolved
tests/validators/answers/test_number_answer_validator.py Show resolved Hide resolved
tests/validators/answers/test_number_answer_validator.py Outdated Show resolved Hide resolved
VirajP1002 and others added 12 commits December 18, 2023 11:06
katie-gardner
katie-gardner reviewed Dec 21, 2023
View reviewed changes
Copy link
Contributor

@katie-gardner katie-gardner left a comment

Choose a reason for hiding this comment

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

A few minor things following the fixes, but looking good!

app/validators/answers/number_answer_validator.py Outdated Show resolved Hide resolved
app/validators/answers/number_answer_validator.py Outdated Show resolved Hide resolved
app/validators/routing/types.py Show resolved Hide resolved
app/validators/routing/types.py Outdated Show resolved Hide resolved
tests/validators/answers/test_number_answer_validator.py Outdated Show resolved Hide resolved
tests/validators/answers/test_number_answer_validator.py Outdated Show resolved Hide resolved
tests/validators/answers/test_number_answer_validator.py Outdated Show resolved Hide resolved
tests/validators/routing/test_types.py Outdated Show resolved Hide resolved
katie-gardner
katie-gardner reviewed Dec 22, 2023
View reviewed changes
Copy link
Contributor

@katie-gardner katie-gardner left a comment

Choose a reason for hiding this comment

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

Since we both looked at it, I'll leave to 2 others as the official reviewers, but looking good to me! 🎉

berroar
berroar reviewed Dec 29, 2023
View reviewed changes
tests/validators/routing/test_types.py Show resolved Hide resolved
app/validators/answers/number_answer_validator.py Outdated Show resolved Hide resolved
app/validators/routing/types.py Outdated Show resolved Hide resolved
app/validators/routing/types.py Outdated Show resolved Hide resolved
Yuyuutsu
Yuyuutsu reviewed Jan 17, 2024
View reviewed changes
Copy link
Contributor

@Yuyuutsu Yuyuutsu left a comment

Choose a reason for hiding this comment

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

Changes look good 🎉

@VirajP1002 VirajP1002 merged commit 9b38581 into main Jan 18, 2024
3 checks passed
@VirajP1002 VirajP1002 deleted the support-float-max-or-min branch January 18, 2024 09:38
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@katie-gardner katie-gardner katie-gardner left review comments

@Yuyuutsu Yuyuutsu Yuyuutsu left review comments

@berroar berroar berroar approved these changes

@MebinAbraham MebinAbraham MebinAbraham approved these changes

Assignees
No one assigned
Labels
Enhancement New feature or request
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
5 participants
@VirajP1002 @MebinAbraham @berroar @Yuyuutsu @katie-gardner