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.

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

feat(sdk): upgrade to Pydantic v2 #27

Merged
merged 1 commit into from
Aug 9, 2023

Conversation

IamAbbey
Copy link
Contributor

@IamAbbey IamAbbey commented Jul 10, 2023

V2 Notes

  1. datetime validation now does not include timezone by default
    Custom datatype introduced to for this TimeZoneDateTime.
    from datetime import datetime
    from pydantic import BaseModel
    
    class Foo(BaseModel):
       bar: datetime
    
    print(Foo(bar="1664200289671"))
    #v1: bar=datetime.datetime(2022, 9, 26, 13, 51, 29, 671000, tzinfo=datetime.timezone.utc)
    #v2: bar=datetime.datetime(2022, 9, 26, 13, 51, 29, 671000)

A Fix was made by Pydantic, so datetime now includes timezone info by default.

  1. date validation: the input value
    provided for a date field must have a nonzero time component. For a
    timestamp to parse into a field of type date, the time components must all be zero. Else date_from_datetime_inexact
    error is raised

    • Implication for youtrack_sdk is that some date type have to convert to custom date data type UnixMidDayDate
      e.g DateIssueCustomField
  2. parse_obj renamed to model_validate

  3. parse_obj_as replaced with TypeAdapter

  4. BaseModel config now uses ConfigDict

  5. Need to provide None default value for optional fields

  6. PydanticValueError replaced with PydanticUserError

  7. BaseModel.schema replaced BaseModel.model_json_schema

  8. BaseModel.dict replaced BaseModel.model_dump

  9. BaseModel.construct replaced BaseModel.model_construct

  10. from_unix_seconds was removed in pydantic v2, so the implementation is now
    copied to .helpers

youtrack_sdk/entities.py Outdated Show resolved Hide resolved
youtrack_sdk/exceptions.py Outdated Show resolved Hide resolved
youtrack_sdk/helpers.py Outdated Show resolved Hide resolved
value: Optional[DateTime | StrictStr | StrictInt | StrictFloat]
value: Optional[datetime | StrictStr | StrictInt | StrictFloat] = None

@field_validator("value")
Copy link
Contributor

Choose a reason for hiding this comment

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

Actually I don't understand why you need this in the first place:

https://docs.pydantic.dev/latest/usage/types/datetime/#validation-of-datetime-types

So my suggestion is to meet to discuss this date situation. To me this looks very fishy irrespectively of the code quality of the implementation.

Unfortunately, you didn't explain what the problem here was with the examples in the PR description, but tried to show your solutions assuming that they are correct.

I hope that we can find a better solution here. Could you please prepare for the call, by showing how it worked before on which inputs and how it works now, which makes problems.

Copy link
Contributor

Choose a reason for hiding this comment

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

Explanation from the meeting: in the past this was in the type, here it is necessary, because the validation result depends on another field. Decision: @IamAbbey will check if it's possible to do it in V2 in the type. If not, then this way will be accepted.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I am at the moment still unable to find possible way to do it in v2 to access other field value using Annotated Types or in type.

Copy link
Contributor

Choose a reason for hiding this comment

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

Copy link
Contributor

Choose a reason for hiding this comment

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

Bottom line: maybe they will add it, but until then, we can use model-bound validator.

youtrack_sdk/types.py Outdated Show resolved Hide resolved
youtrack_sdk/types.py Outdated Show resolved Hide resolved
youtrack_sdk/exceptions.py Outdated Show resolved Hide resolved
@IamAbbey
Copy link
Contributor Author

New fixup commits added to address open suggestions

@IamAbbey IamAbbey requested review from zyv and catcombo July 18, 2023 08:00
@zyv
Copy link
Contributor

zyv commented Jul 18, 2023

/rebase

@mergealot mergealot force-pushed the feature/migrate-to-pydantic-v2 branch from 16a7d66 to 9b92d9c Compare July 18, 2023 10:29
youtrack_sdk/types.py Outdated Show resolved Hide resolved
youtrack_sdk/entities.py Outdated Show resolved Hide resolved
youtrack_sdk/entities.py Outdated Show resolved Hide resolved
youtrack_sdk/entities.py Outdated Show resolved Hide resolved
tests/test_definitions.py Outdated Show resolved Hide resolved
tests/test_definitions.py Outdated Show resolved Hide resolved
Copy link
Contributor

@zyv zyv left a comment

Choose a reason for hiding this comment

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

I've pushed a WIP, in as far as I'm concerned that's fine now.

@IamAbbey IamAbbey force-pushed the feature/migrate-to-pydantic-v2 branch from 1a4275e to 4458b2e Compare July 31, 2023 10:13
@IamAbbey
Copy link
Contributor Author

/rebase

@mergealot mergealot force-pushed the feature/migrate-to-pydantic-v2 branch from 4458b2e to 6528cff Compare July 31, 2023 10:13
@IamAbbey IamAbbey requested a review from catcombo August 1, 2023 07:15
Copy link
Contributor

@catcombo catcombo left a comment

Choose a reason for hiding this comment

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

Tested 👍

@IamAbbey
Copy link
Contributor Author

IamAbbey commented Aug 9, 2023

/rebase

@mergealot mergealot force-pushed the feature/migrate-to-pydantic-v2 branch from a64d665 to d79066d Compare August 9, 2023 10:52
@IamAbbey IamAbbey merged commit f4382fe into master Aug 9, 2023
4 checks passed
@IamAbbey IamAbbey deleted the feature/migrate-to-pydantic-v2 branch August 9, 2023 11:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

3 participants