-
Notifications
You must be signed in to change notification settings - Fork 35
migrate to pydantic v2 #1535
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
migrate to pydantic v2 #1535
Conversation
| @validator('protocol_repeats') | ||
| def must_be_positive(cls, v): | ||
| if v <= 0: | ||
| errmsg = f"protocol_repeats must be a positive value, got {v}." | ||
| raise ValueError(errmsg) | ||
| return v | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't feel strongly about using Annotated[int, Gt(0)] over this.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What error do you get with Annotated[int, Gt(0)]. The advantage I could see of sticking with the validator is to give folks good error messages on why it won't work. That's quite important with settings.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
the error message would be:
pydantic_core._pydantic_core.ValidationError: 1 validation error for PlainMDProtocolSettings
protocol_repeats
Input should be greater than 0 [type=greater_than, input_value=-1, input_type=int] For further information visit https://errors.pydantic.dev/2.11/v/greater_thanI think this is less helpful than the validator, so I'll revert for now. However, this type of pydantic error will be used elsewhere.
We should have a follow-up PR that customizes error messages (maybe something like this), but I consider that out of scope of this PR.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
reverted, and issue opened in gufe: OpenFreeEnergy/gufe#641
| @validator('protocol_repeats') | ||
| def must_be_positive(cls, v): | ||
| if v <= 0: | ||
| errmsg = f"protocol_repeats must be a positive value, got {v}." | ||
| raise ValueError(errmsg) | ||
| return v | ||
|
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What error do you get with Annotated[int, Gt(0)]. The advantage I could see of sticking with the validator is to give folks good error messages on why it won't work. That's quite important with settings.
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## main #1535 +/- ##
==========================================
- Coverage 95.25% 93.26% -2.00%
==========================================
Files 172 172
Lines 14492 14482 -10
==========================================
- Hits 13805 13506 -299
- Misses 687 976 +289
Flags with carried forward coverage won't be shown. Click here to find out more. ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
Co-authored-by: Irfan Alibay <IAlibay@users.noreply.github.com>
Co-authored-by: Irfan Alibay <IAlibay@users.noreply.github.com>
|
No API break detected ✅ |
resolves OpenFreeEnergy/gufe#585
(new PR because I messed up #1431)