-
Notifications
You must be signed in to change notification settings - Fork 27
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
⬆️ Upgrade libraries (pydantic v2) #6366
⬆️ Upgrade libraries (pydantic v2) #6366
Conversation
Since Pydantic v2, ByteSize are validated and must be >= 0.
Quality Gate passedIssues Measures |
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.
Feel free to merge after you have addressed my comments. Looks good to me.
- please refactor by moving to
TypeAdapterSomeClass
all the most used functions and import them from common_library. I have left a few comments regarding these - there are a few checks that I as you to run if all are OK, go ahead an merge
Thanks a lot for the effort. It's looking good.
min_length=0, | ||
max_length=256, |
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 these constants as well here
AWS_TAG_KEY_MIN_LENGTH: Final[int] = 1 | ||
AWS_TAG_KEY_MAX_LENGTH: Final[int] = 128 |
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 would dlclare them at the top of the file as it is common convention in python
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.
Yes, forgot there :)
AWS_TAG_VALUE_MIN_LENGTH: Final[int] = 0 | ||
AWS_TAG_VALUE_MAX_LENGTH: Final[int] = 256 |
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.
declare on the top of the file
@@ -49,6 +49,8 @@ | |||
from types_aiobotocore_s3 import S3Client | |||
from types_aiobotocore_s3.literals import BucketLocationConstraintType | |||
|
|||
_BYTE_SIZE_ADAPTER: Final[TypeAdapter[ByteSize]] = TypeAdapter(ByteSize) |
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 use the lowercase way of declaring it TypeAdapterByteSize or import it from a common library since this one is used quite a lot in the entire code base
@@ -67,7 +69,9 @@ async def simcore_s3_api( | |||
@pytest.fixture | |||
def bucket_name(faker: Faker) -> S3BucketName: | |||
# NOTE: no faker here as we need some specific namings | |||
return parse_obj_as(S3BucketName, faker.pystr().replace("_", "-").lower()) | |||
return TypeAdapter(S3BucketName).validate_python( |
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.
would declare this one in the context of the file, in Lowe case like TypeAdapterS3BucketName
|
||
|
||
class TwilioSettings(BaseCustomSettings): | ||
TWILIO_ACCOUNT_SID: str = Field(..., description="Twilio account String Identifier") | ||
TWILIO_AUTH_TOKEN: str = Field(..., description="API tokens") | ||
TWILIO_COUNTRY_CODES_W_ALPHANUMERIC_SID_SUPPORT: list[CountryCodeStr] = Field( | ||
default=parse_obj_as( | ||
list[CountryCodeStr], | ||
default=TypeAdapter(list[CountryCodeStr]).validate_python( |
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.
extract this in this module at the top for performance reasons
base_url = TypeAdapter(AnyHttpUrl).validate_python(settings_with_defaults.base_url) | ||
api_base_url = TypeAdapter(AnyHttpUrl).validate_python(settings_with_defaults.api_base_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.
e from common can be used
@@ -218,8 +217,8 @@ def e_tag_fixture() -> str: | |||
async def mock_filemanager(mocker: MockerFixture, e_tag: str, faker: Faker) -> None: | |||
mocker.patch( | |||
"simcore_sdk.node_ports_common.filemanager._get_file_meta_data", | |||
return_value=parse_obj_as( | |||
FileMetaDataGet, FileMetaDataGet.Config.schema_extra["examples"][0] | |||
return_value=TypeAdapter(FileMetaDataGet).validate_python( |
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 common one can be used
abort_upload=TypeAdapter(AnyUrl).validate_python(faker.uri()), | ||
complete_upload=TypeAdapter(AnyUrl).validate_python(faker.uri()), |
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.
use the common one
TypeAdapter(ByteSize).validate_python(21800510238), | ||
TypeAdapter(ByteSize).validate_python(10485760), |
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.
e you might have a common one
1d92881
into
ITISFoundation:pydantic_v2_migration
What do these changes do?
Upgrade libraries (inside
packages
folder) to Pydantic v2.Note: the
models-library
has a dedicated PR (see #6333)Related issue/s
How to test
Dev-ops checklist