-
Notifications
You must be signed in to change notification settings - Fork 14
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
195 migrate to pydantic v2 #199
Conversation
@djs0109 I have to look into it. |
@djs0109 did you run the migration tool on the tests as well? Otherwise I will add it very quickly |
Yes, I run it already. These parts are not automatically migrated. I guess the reason is that they are not inherited from the |
@djs0109 I will need a little more time. I will go ahead but will require a little for it. The trouble is probably in pydantic-settings but pydantiv itself. |
@djs0109 Hi, i looked into it bit. I think that regex expressions are still an issue. There is an open issue in |
… 195-Migrate-to-pydantic-v2
The migration is mostly done. All tests pass also in CI-runner. @tstorek do you have time to review it? |
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.
Hi, thanks for that big effort. I found some stuff. Please double check on them
filip/clients/ngsi_v2/cb.py
Outdated
@@ -385,9 +386,11 @@ def get_entity_list(self, | |||
params=params, | |||
headers=headers) | |||
if AttrsFormat.NORMALIZED in response_format: | |||
return parse_obj_as(List[ContextEntity], items) | |||
ta = TypeAdapter(List[ContextEntity]) |
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.
Please follow the notation in the migration guide: https://docs.pydantic.dev/2.0/migration/#introduction-of-typeadapter
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 change the variable name from ta
to adapter
just as in the documentation.
filip/clients/ngsi_v2/iota.py
Outdated
@@ -92,7 +93,7 @@ def post_groups(self, | |||
|
|||
url = urljoin(self.base_url, 'iot/services') | |||
headers = self.headers | |||
data = {'services': [group.dict(exclude={'service', 'subservice'}, | |||
data = {'services': [group.model_dump(exclude={'service', 'subservice'}, | |||
exclude_none=True, |
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.
adapt indentation
filip/config.py
Outdated
env_file_encoding = 'utf-8' | ||
case_sensitive = False | ||
validation_alias=AliasChoices('QUANTUMLEAP_URL', 'QL_URL')) | ||
model_config = SettingsConfigDict(env_file='.env.filip', env_file_encoding='utf-8', |
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.
consider to move to the top of the class. To make it easier to access
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.
Can you describe more about this point? I could not get your suggestion
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.
Model config should be directly below the class doc-string to have a better overview about whats going on
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.
It is already fixed
filip/models/ngsi_v2/base.py
Outdated
@@ -25,7 +27,8 @@ class Http(BaseModel): | |||
"also be supported." | |||
) | |||
|
|||
@validator('url', allow_reuse=True) | |||
@field_validator('url') | |||
@classmethod | |||
def check_url(cls, value): | |||
return validate_http_url(url=value) |
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.
Does this stil work with the new Url-pattern?
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.
Maybe use the new validation pattern for reusing the validator
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 new URL-pattern do you mean?
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.
Check on here: This seems far easier:) https://docs.pydantic.dev/latest/api/networks/
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.
If I replace L23 with url: HttpUrl
or url: AnyHttpUrl
, the http
data can not be passed into the data model with the syntax Subscription(**sub_dict)
That is a good hint 😄
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.
Hence, to change or not?
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.
It is already updated, please check the latest commit
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.
@tstorek Thanks for the reviewing effort! I went through your comments. Please check again, if it looks ok for you
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.
@djs0109 There are jsut minor things. I prefer to have the model config as first entry a pydantic model. That's what I orginally meant with my comment
filip/models/ngsi_v2/base.py
Outdated
@@ -25,7 +27,8 @@ class Http(BaseModel): | |||
"also be supported." | |||
) | |||
|
|||
@validator('url', allow_reuse=True) | |||
@field_validator('url') | |||
@classmethod | |||
def check_url(cls, value): | |||
return validate_http_url(url=value) |
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.
Check on here: This seems far easier:) https://docs.pydantic.dev/latest/api/networks/
@tstorek sure! I have now adjusted the location of |
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.
only minor things. Max 1h of work
filip/config.py
Outdated
env_file_encoding = 'utf-8' | ||
case_sensitive = False | ||
validation_alias=AliasChoices('QUANTUMLEAP_URL', 'QL_URL')) | ||
model_config = SettingsConfigDict(env_file='.env.filip', env_file_encoding='utf-8', |
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.
Model config should be directly below the class doc-string to have a better overview about whats going on
filip/models/ngsi_v2/base.py
Outdated
@@ -25,7 +27,8 @@ class Http(BaseModel): | |||
"also be supported." | |||
) | |||
|
|||
@validator('url', allow_reuse=True) | |||
@field_validator('url') | |||
@classmethod | |||
def check_url(cls, value): | |||
return validate_http_url(url=value) |
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.
Hence, to change or not?
filip/models/ngsi_v2/iot.py
Outdated
default=None, | ||
description="Endpoint where the device is going to receive commands, " | ||
"if any." | ||
) | ||
@field_validator('endpoint') | ||
@classmethod | ||
def validate_endpoint(cls, value): |
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.
@@ -22,10 +25,10 @@ def validate_http_url(url: AnyHttpUrl) -> str: | |||
Returns: | |||
validated url | |||
""" | |||
return url | |||
return str(url) if url else url |
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.
https://docs.pydantic.dev/latest/api/networks/ this doesn't work?
@djs0109 i'm sorry somehow the latest commit did not show up |
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.
Whoop Whoop. Thanks a lot! Let#s go for it. Don#t forget the Changelog ;)
@tstorek I met the first difficulty while doing the migration. Previously, you defined the
QueryStatement
andQueryStatement
insimple_ql.py
, and now they cause the issues:pydantic.errors.PydanticSchemaGenerationError: Unable to generate pydantic-core schema for <class 'filip.utils.simple_ql.QueryString'>
. Do you have an Idea about how to fix that?