Skip to content

fix: skip truncate_datetime for date and timedelta objects - #617

Open
Sanjays2402 wants to merge 1 commit into
qlustered:masterfrom
Sanjays2402:fix/truncate-datetime-date-timedelta
Open

Sanjays2402 wants to merge 1 commit into
qlustered:masterfrom
Sanjays2402:fix/truncate-datetime-date-timedelta

Conversation

@Sanjays2402

Copy link
Copy Markdown

Fixes #564

datetime_normalize applied truncate_datetime to every value routed through it, so DeepDiff(..., truncate_datetime=...) raised TypeError: replace() got an unexpected keyword argument 'microsecond' for datetime.date values and AttributeError for datetime.timedelta values. date has no sub-day components to truncate and timedelta has no replace(), so the truncation is now applied only to datetime and time objects; truncation of those is unchanged.

Regression test added in tests/test_diff_datetime.py: it fails on master with the reported TypeError and passes with the fix.

This change was prepared with AI assistance; the regression test was run locally and fails without the fix.

DeepDiff(..., truncate_datetime=...) raised TypeError for datetime.date
values ('replace() got an unexpected keyword argument microsecond') and
AttributeError for datetime.timedelta values, because datetime_normalize
applied the truncation unconditionally to every value routed through it.

date has no time components to truncate and timedelta has no replace(),
so the truncation is now applied only to datetime and time objects.
Truncation of datetime/time values is unchanged.

Fixes qlustered#564
@codecov-commenter

Copy link
Copy Markdown

⚠️ Please install the 'codecov app svg image' to ensure uploads and comments are reliably processed by Codecov.

Codecov Report

✅ All modified and coverable lines are covered by tests.
✅ Project coverage is 95.10%. Comparing base (79e4379) to head (c483de1).
❗ Your organization needs to install the Codecov GitHub app to enable full functionality.

Additional details and impacted files
@@           Coverage Diff           @@
##           master     #617   +/-   ##
=======================================
  Coverage   95.10%   95.10%           
=======================================
  Files          17       17           
  Lines        4900     4900           
=======================================
  Hits         4660     4660           
  Misses        240      240           

☔ View full report in Codecov by Harness.
📢 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.

@seperman seperman left a comment

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

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

Static review: #617 fix: skip truncate_datetime for date and timedelta objects

Summary: Makes datetime_normalize apply truncate_datetime only to datetime.datetime and datetime.time objects. This fixes the date crash reported in #564. It also fixes a remaining crash for timedelta: _diff_time sends timedelta values to datetime_normalize, which calls .replace() on them.

Overlap with other PRs

  • The date part of #564 is already fixed on dev by #597 (merged). #597 adds a has_time check at helper.py:694-696 (on dev) that skips truncation only for plain date. #602 was an earlier fix for the same issue; it was closed because the fix is already on dev.
  • What this PR adds beyond #597:
    • timedelta + truncate_datetime: on dev, has_time is True for timedelta, so _diff_time (diff.py:1993-1997 on dev) still calls timedelta.replace(...) and raises AttributeError. This PR fixes that.
    • np.datetime64 under truncate_datetime in DeepHash: it reaches datetime_normalize through DeepHash._prep_datetime, since it is in times. As far as I know np.datetime64 has no .replace(), so it should also raise on dev. With this PR it would be passed through without truncation. That avoids the crash but silently ignores the option for np.datetime64.
  • The PR targets master, and its change at helper.py:693 conflicts textually with #597's change on dev.
  • No overlap with #635. That PR changes number_to_string, a different code path. Both PRs append to tests/test_diff_datetime.py and change its datetime import line, so a small rebase will be needed.

Security

No security concerns found in static review. The diff is a one-line condition change plus a test. There is no I/O and no dependency or CI change, and the diff has no non-ASCII or hidden characters.

Correctness

  • deepdiff/helper.py:693: the positive isinstance(obj, (datetime.datetime, datetime.time)) check is tighter than dev's negative has_time check, because it covers date, timedelta and np.datetime64 together. datetime subclasses (e.g. pandas Timestamp) are still truncated as before. time truncation is unchanged.
  • Skipping truncation for timedelta is consistent with the docs, which describe truncate_datetime as applying to datetime objects. Whether truncate_datetime='second' should also ignore sub-second timedelta differences is a separate design question. After this PR such values are compared exactly.

Tests

  • tests/test_diff_datetime.py:127: the date assertions already pass on dev (since #597) and repeat test_truncate_datetime_with_date in tests/test_diff_other.py. The timedelta assertion goes through datetime_normalize and should fail on dev with AttributeError, so it is the part of the test that adds coverage.
  • There's no equal-timedelta case (e.g. timedelta(days=1) vs timedelta(days=1) returns {}).

Suggestions

  • Retarget to dev and rebase. When resolving the conflict at helper.py:693, replace #597's has_time lines with this positive check.
  • Keep the timedelta test and drop the duplicated date assertions, or move them next to #597's test. Consider an np.datetime64 DeepHash case such as DeepHash(np.datetime64('2020-05-17T22:15:30'), truncate_datetime='minute') to document the chosen behaviour.

AI-assisted static review; the code was not executed.

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.

'second' is an invalid keyward argument for replace() when using truncate_datetime and a date object

3 participants