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

#1165: Fixing and Testing "Duration.diff returns negative number" #1186

Open
wants to merge 2 commits into
base: master
Choose a base branch
from

Conversation

JoaoAugustoPansani
Copy link

On the file ./scripts/src/impl/diff.js there's a function dayDiff:

function dayDiff(earlier, later) {
  const utcDayStart = (dt) => dt.toUTC(0, { keepLocalTime: true }).startOf("day").valueOf(),
    ms = utcDayStart(later) - utcDayStart(earlier);
  return Math.floor(Duration.fromMillis(ms).as("days"));
}

What I did for solving this issue, was delete the second argument inside .toUTC:

function dayDiff(earlier, later) {
  const utcDayStart = (dt) => dt.toUTC(0).startOf("day").valueOf(),
    ms = utcDayStart(later) - utcDayStart(earlier);
  return Math.floor(Duration.fromMillis(ms).as("days"));
}

Then I wrote a test inside ./scripts/test/datetime/diff.test.js:

test("DateTime#diff works when date has a zone object", () => {
  const start = DateTime.fromISO("2022-05-05T23:00", { zone: "UTC" });
  const end = DateTime.fromISO("2022-05-10T00:00", { zone: "Europe/Madrid" });

  const diff = end.diff(start, ["days", "hours", "minutes"]).toObject();
  expect(diff.days).toBe(3);
  expect(diff.hours).toBe(23);
  expect(diff.minutes).toBe(0);
});

@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Apr 21, 2022

CLA Signed

The committers listed above are authorized under a signed CLA.

  • ✅ login: JoaoAugustoPansani / name: João Augusto Mendonça Pansani (c55a8be, 65124da)

@icambron
Copy link
Member

icambron commented May 8, 2022

Sorry for the slow response. I don't think this fix is correct, though. That second argument ensures that diff compares local times, not absolute ones. You can see the problem by trying these dates instead:

const start = DateTime.fromISO("2022-05-06T00:00", { zone: "UTC" });
const end = DateTime.fromISO("2022-05-10T00:00", { zone: "Europe/Madrid" });

dayDiff(start, end) // returns 3, not 4

@JoaoAugustoPansani
Copy link
Author

No problem @icambron, that's fine. I'll work on that, thanks for the response!

@omasback
Copy link

omasback commented Jun 2, 2022

Sorry I've only briefly looked at this but why not just convert both datetimes toMillis() and subtract? Then convert millis to Duration and shiftTo() back to requested units?

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.

3 participants