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

Fix TzInfo equality check based on offset #1197

Merged
merged 9 commits into from
Feb 20, 2024

Conversation

13sin
Copy link
Contributor

@13sin 13sin commented Feb 18, 2024

Change Summary

comparison check doesn't works with datatime.tzinfo derived class- timezone. In this change, the comparison is based on offset set(in seconds) for tzinfo class.

Related issue number

fix pydantic/pydantic#8683
Also partially fixes pydantic/pydantic#8195, in this the issue is that comparison check for different tzinfo fails.

  • In case of utc there is instancecheck and identity check with is.
  • but in case of other timezones it does equality check. link

Checklist

  • Unit tests for the changes exist
  • Documentation reflects the changes where applicable
  • Pydantic tests pass with this pydantic-core (except for expected changes)
  • My PR is ready to review, please add a comment including the phrase "please review" to assign reviewers

Selected Reviewer: @adriangb

Copy link

codecov bot commented Feb 18, 2024

Codecov Report

Merging #1197 (f099f3a) into main (1fb3343) will increase coverage by 90.25%.
Report is 12 commits behind head on main.
The diff coverage is 100.00%.

Additional details and impacted files
@@            Coverage Diff            @@
##           main    #1197       +/-   ##
=========================================
+ Coverage      0   90.25%   +90.25%     
=========================================
  Files         0      106      +106     
  Lines         0    16352    +16352     
  Branches      0       36       +36     
=========================================
+ Hits          0    14758    +14758     
- Misses        0     1587     +1587     
- Partials      0        7        +7     
Files Coverage Δ
src/input/datetime.rs 98.79% <100.00%> (ø)

... and 105 files with indirect coverage changes


Continue to review full report in Codecov by Sentry.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update ea443ba...f099f3a. Read the comment docs.

Copy link

codspeed-hq bot commented Feb 18, 2024

CodSpeed Performance Report

Merging #1197 will improve performances by 24.38%

Comparing 13sin:fix-TzInfo-Equality-Check (f099f3a) with main (ea443ba)

Summary

⚡ 3 improvements
✅ 145 untouched benchmarks

Benchmarks breakdown

Benchmark main 13sin:fix-TzInfo-Equality-Check Change
test_core_future_str 39.4 µs 32.6 µs +20.91%
test_core_str 39.1 µs 32 µs +22.22%
test_core_future 37.7 µs 30.3 µs +24.38%

@13sin
Copy link
Contributor Author

13sin commented Feb 18, 2024

please review

Copy link
Contributor

@davidhewitt davidhewitt left a comment

Choose a reason for hiding this comment

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

Thanks! A few additional thoughts from me...

src/input/datetime.rs Outdated Show resolved Hide resolved
src/input/datetime.rs Show resolved Hide resolved
estdatetime = self.DT.replace(tzinfo=timezone(-timedelta(hours=5)))
self.assertTrue(self.EST == estdatetime.tzinfo)
self.assertTrue(tz > estdatetime.tzinfo)

Copy link
Contributor

Choose a reason for hiding this comment

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

I think we should add tests to confirm that comparisons with zoneinfo.ZoneInfo("Europe/London") or similar don't succeed.

Copy link
Contributor Author

@13sin 13sin Feb 20, 2024

Choose a reason for hiding this comment

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

Test added for zoneinfo.ZoneInfo would check whether it return false. As, NotImplemented would not throw error. there would be identity check eventually as __eq__ is not implemented for right-side and left-side. Would this be right way of doing this? Other option would be to throw NotImplementedError.

Copy link
Contributor

Choose a reason for hiding this comment

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

I think by returning NotImplemented then equality will return False and ordering operators will raise a TypeError. I see you added an equality test, maybe add a test that ordering throws TypeError and then we're good here?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Got it. Thank you so much @davidhewitt .

Copy link
Contributor

@davidhewitt davidhewitt 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!

tests/test_tzinfo.py Show resolved Hide resolved
@davidhewitt davidhewitt enabled auto-merge (squash) February 20, 2024 10:38
@davidhewitt davidhewitt merged commit c2c90fd into pydantic:main Feb 20, 2024
26 checks passed
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.

Pydantic TzInfo(UTC) != datetime.UTC v2 TZInfo is not understood by pandas
3 participants