Skip to content

Conversation

@Viicos
Copy link
Contributor

@Viicos Viicos commented Nov 2, 2023

This way zoneinfo or third party libraries can be used as well.
Related: HypothesisWorks/hypothesis#3780

Copy link
Contributor

@adriangb adriangb left a comment

Choose a reason for hiding this comment

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

Is this backwards compatible?

@Viicos
Copy link
Contributor Author

Viicos commented Nov 2, 2023

Is this backwards compatible?

Currently only timezone objects are allowed, and timezone is a subclass of tzinfo so I'd say yes it is.

Also, should I add a Changed in 0.x.x in the readme?

@adriangb
Copy link
Contributor

adriangb commented Nov 2, 2023

Yes please add the changed section

@codecov-commenter
Copy link

codecov-commenter commented Nov 2, 2023

Codecov Report

Merging #56 (a1728fa) into main (18584df) will not change coverage.
The diff coverage is 100.00%.

❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

@@           Coverage Diff           @@
##             main      #56   +/-   ##
=======================================
  Coverage   94.76%   94.76%           
=======================================
  Files           2        2           
  Lines         210      210           
  Branches       30       30           
=======================================
  Hits          199      199           
  Misses          9        9           
  Partials        2        2           
Files Coverage Δ
annotated_types/__init__.py 92.25% <100.00%> (ø)

📣 We’re building smart automated test selection to slash your CI/CD build times. Learn more

Copy link
Member

@Zac-HD Zac-HD left a comment

Choose a reason for hiding this comment

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

I think this is basically a documentation fix rather than a new feature, but let's just merge it instead of debating how to describe it. Thanks @Viicos for noticing and fixing the problem!

@Zac-HD Zac-HD merged commit 657ded9 into annotated-types:main Nov 2, 2023
@Viicos Viicos deleted the timezone branch November 2, 2023 15:22
@adriangb
Copy link
Contributor

@Viicos or @Zac-HD should we additionally or instead support integer second offsets? Pydantic can't really validate against string time zones, at least not easily.

cc @sydney-runkle

@Zac-HD
Copy link
Member

Zac-HD commented May 20, 2024

Per pydantic/pydantic#8088 (comment), IMO we should not.

Offsets are a fundamentally different datatype than timezones: the UTC offset of a timezone changes with every daylight savings transition, as well as at other times when the tz rules are changed.

Furthermore, offsets are very very very rarely the right solution to a problem, making them an attractive nuisance which causes far more trouble in bugs than it saves in the few cases where it's the right way. I think you can cover most reasonable cases with ZoneInfo.key, treating a zero-utc-offset tzinfo as UTC, and requiring users to write custom validators for other cases.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants