Skip to content

Add fold to the datetime fieldtype to resolve datetime ambiguity#217

Merged
yunzheng merged 9 commits intomainfrom
make-datetime-fold
Mar 11, 2026
Merged

Add fold to the datetime fieldtype to resolve datetime ambiguity#217
yunzheng merged 9 commits intomainfrom
make-datetime-fold

Conversation

@Miauwkeru
Copy link
Contributor

This is for disambiguation during dst time

@codecov
Copy link

codecov bot commented Mar 11, 2026

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 84.23%. Comparing base (a30f10d) to head (4925eb1).
⚠️ Report is 1 commits behind head on main.

Additional details and impacted files
@@           Coverage Diff           @@
##             main     #217   +/-   ##
=======================================
  Coverage   84.23%   84.23%           
=======================================
  Files          35       35           
  Lines        3742     3742           
=======================================
  Hits         3152     3152           
  Misses        590      590           
Flag Coverage Δ
unittests 84.23% <ø> (ø)

Flags with carried forward coverage won't be shown. Click here to find out more.

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

🚀 New features to boost your workflow:
  • ❄️ Test Analytics: Detect flaky tests, report on failures, and find test suite problems.

@Miauwkeru
Copy link
Contributor Author

Something I noticed when writing fox-it/dissect.target#1614
The fold argument isn't passed through

@yunzheng yunzheng self-assigned this Mar 11, 2026
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

This PR addresses DST ambiguity by preserving the datetime.fold attribute when flow.record.fieldtypes.datetime is constructed from an existing datetime instance, and adds tests to validate correct behavior around DST transitions.

Changes:

  • Preserve fold when converting from a datetime object inside the datetime fieldtype.
  • Extend datetime fieldtype tests to cover fold=1 and an end-of-DST scenario using Europe/Amsterdam.

Reviewed changes

Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.

File Description
flow/record/fieldtypes/__init__.py Propagates arg.fold when wrapping an existing datetime into the fieldtype.
tests/fieldtypes/test_fieldtypes.py Adds parametrized tests and an explicit DST fold example using ZoneInfo.

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

You can also share your feedback on Copilot code review. Take the survey.

yunzheng and others added 2 commits March 11, 2026 22:11
Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

You can also share your feedback on Copilot code review. Take the survey.

Copy link

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull request overview

Copilot reviewed 2 out of 2 changed files in this pull request and generated 3 comments.


💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

You can also share your feedback on Copilot code review. Take the survey.

Copy link
Member

@yunzheng yunzheng left a comment

Choose a reason for hiding this comment

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

LGTM, added an additional test and skip tests if zoneinfo is not available.

@yunzheng yunzheng merged commit a594c19 into main Mar 11, 2026
23 checks passed
@yunzheng yunzheng deleted the make-datetime-fold branch March 11, 2026 22:29
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.

3 participants