Skip to content

Conversation

Youssefares
Copy link
Contributor

@Youssefares Youssefares commented Jul 15, 2024

Change Summary

Related issue number

fix #4710

Checklist

  • The pull request title is a good summary of the changes - it will be used in the changelog
  • Unit tests for the changes exist
  • Tests pass on CI
  • Documentation reflects the changes where applicable
  • My PR is ready to review, please add a comment including the phrase "please review" to assign reviewers

Selected Reviewer: @sydney-runkle

@github-actions github-actions bot added the relnotes-fix Used for bugfixes. label Jul 15, 2024
Copy link

codspeed-hq bot commented Jul 15, 2024

CodSpeed Performance Report

Merging #9896 will not alter performance

Comparing Youssefares:types/zoneinfo (57e85b7) with main (7cf8239)

Summary

✅ 14 untouched benchmarks

@yezz123
Copy link
Contributor

yezz123 commented Jul 15, 2024

hey @Youssefares correct me if i'm wrong but i guess its similar to this one we have in Pydantic-extra pydantic/pydantic-extra-types#193

@Youssefares
Copy link
Contributor Author

Youssefares commented Jul 15, 2024

hey @Youssefares correct me if i'm wrong but i guess its similar to this one we have in Pydantic-extra pydantic/pydantic-extra-types#193

This does seem very similar, zoneinfo.ZoneInfo is part of the python std library though. I maybe wrong but my understanding of pydantic-extra-types is that it defines additional convenience types, while pydantic itself is where we offer support for standard types. I am not 100% sure about the need for an extra-type in this scenario given zoneinfo now exists since 3.9. I also think (in my experience at least), most people try to drop the pytz dependency from their project nowadays if zoneinfo does the job.

I'll leave to maintainers to decide whether the extra type using pytz is still required, but I believe supporting zoneinfo is very much worthwhile eitherway.

@yezz123
Copy link
Contributor

yezz123 commented Jul 15, 2024

hey @Youssefares correct me if i'm wrong but i guess its similar to this one we have in Pydantic-extra pydantic/pydantic-extra-types#193

This does seem very similar, zoneinfo.ZoneInfo is part of the python std library though. I maybe wrong but my understanding of pydantic-extra-types is that it defines additional convenience types, while pydantic itself is where we offer support for standard types. I am not 100% sure about the need for an extra-type in this scenario given zoneinfo now exists since 3.9. I also think (in my experience at least), most people try to drop the pytz dependency from their project nowadays if zoneinfo does the job.

I'll leave to maintainers to decide whether the extra type using pytz is still required, but I believe supporting zoneinfo is very much worthwhile eitherway.

Yes as you said the best answer we can have is from maintainers themselves.

cc @sydney-runkle @samuelcolvin

@Youssefares
Copy link
Contributor Author

please review

@sydney-runkle
Copy link
Contributor

Exciting stuff here - this is next in my queue to review :). Should be able to get to it this evening or first thing tomorrow :).

Copy link
Contributor

@sydney-runkle sydney-runkle left a comment

Choose a reason for hiding this comment

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

Very excited to include this. Added one minor simplification request!

@pydantic-hooky pydantic-hooky bot added awaiting author revision awaiting changes from the PR author and removed ready for review labels Jul 22, 2024
@sydney-runkle
Copy link
Contributor

I'll leave to maintainers to decide whether the extra type using pytz is still required, but I believe supporting zoneinfo is very much worthwhile eitherway.

I think we should definitely just stick to std lib support, so the zoneinfo addition here is great!

@Youssefares
Copy link
Contributor Author

CI's failing because this issue #9761 been resurrected somehow it seems? Looking into it..

@Youssefares
Copy link
Contributor Author

Youssefares commented Jul 24, 2024

That looks better.. 🟢 ✅

Copy link
Contributor

@sydney-runkle sydney-runkle left a comment

Choose a reason for hiding this comment

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

Looks great!

Thanks a bunch!

@sydney-runkle sydney-runkle merged commit c94049b into pydantic:main Jul 24, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Add timezone to tzinfo parsing to pydantic 2.0
3 participants