-
Notifications
You must be signed in to change notification settings - Fork 23
Migrate fully to pydantic 2 #268
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 fully to pydantic 2 #268
Conversation
I don't remember clearly, but I wonder if that was because v1 had some limitations. I'm not sure, thou. But please do the modifications which you think makes sense. BTW! There is a feature we don't support at all, and that's pagination |
d3d46ea
to
921e9df
Compare
I've added some more values, though I think maybe we should branch out a separate issue on what to do with validation going forward. Key questions (off the top of my head):
|
The purpose of protocol validation is only to check the data so that api.py doesn't crash if given incorrect data. Obviously we don't control the protocol and exclusively in Zaptec hands, so it might change without our notice. If we make the net too tight, it'll end up unexpected on our table as a non-working integration if and when that happens. With regards to pagination its mentioned https://docs.zaptec.com/docs/api-usage-guidelines#5-real-time-updates-and-efficiency and https://docs.zaptec.com/page/api-changes-what-you-need-to-know#2-updated-pagination-limits-for-better-performance. I know of no HA user with more than 50 chargers, but it only takes a decision by Zaptec to lower this to levels where its important to consider....but we can chose to be reactive to a change like that. I think I'm happy with how it is for now, but we shouldn't forget. |
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.
A general comment: How much should be included into the validation? Should we validate the fields that are being used in the code, or should we list everything that's returned from Zaptec?
For the current implementation, the goal has been the former: Only validate the fields which are necessary for using the api, and ignore the rest.
If the latter is chosen, do we have a reliable source for the schema we can use? I think it would be a pity if Zaptec updates their schema and we did not, and the integration fails for users on a field that we're not even using. But I'm kind of opinionated here, so I'd like to hear your opinion.
I agree that we should stick to validating the fields that are necessary for the integration to work properly. As long as Zaptec doesn't change something we rely on, the integration shouldn't fail. Sidenote: I think #283 is severe enough that we should move this PR and several other topics that are still open from the v0.8.2-milestone to the next, and get 0.8.2(beta?) out asap. |
921e9df
to
81ad18d
Compare
Should we bump |
Sure, thoughts on how restrictive we want to be? |
I think your proposal of |
Fixes custom-components#267 There were a couple of attributes that were commented out for some reason. Have set these as optional (`type | None = None`) for the time being, though I'm not sure why these were commented out in the first place. Should we make these attributes (and/or others) required in addition to the ones we already require, or should we leave it as-is?
More strict homeassistant-requirement, less strict pydantic requirement Also update ruff and homeassistant in dev-container to latest versions
81ad18d
to
32bebe7
Compare
LGTM. We need to remember to include a comment in the next release notes that the minimum HA version has been increased. |
Fixes #267
There were a couple of attributes that were commented out for some reason. Have set these as optional (
type | None = None
) for the time being, though I'm not sure why these were commented out in the first place.Should we make these attributes (and/or others) required in addition to the ones we already require, or should we leave it as-is?