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

alternative fix to #1082 #1083

Merged
merged 4 commits into from
Dec 8, 2021
Merged

alternative fix to #1082 #1083

merged 4 commits into from
Dec 8, 2021

Conversation

icambron
Copy link
Member

No description provided.

@linux-foundation-easycla
Copy link

linux-foundation-easycla bot commented Nov 16, 2021

CLA Signed

The committers are authorized under a signed CLA.

@brendan-gero-humanetix
Copy link

I know I said I'd be happy with this as is, but a thought just occurred to me. Could we add one more test case?

Something along the lines of

test("DateTime.fromFormat() maintains offset that belongs to time zone during overlap", () => {
  // On this day, 02:30 does not exist, due to DST starting.
  let i = DateTime.fromFormat(
    "2021-10-03T02:30:00.000+10:00[Australia/Sydney]",
    "yyyy-MM-dd'T'HH:mm:ss.SSSZZ[z]",
    { setZone: true }
  );
  expect(i.isValid).toBe(true);
  expect(i.year).toBe(2021);
  expect(i.month).toBe(4);
  expect(i.day).toBe(4);
  expect(i.hour).toBe(3);
  expect(i.minute).toBe(30);
  expect(i.second).toBe(0);
  expect(i.millisecond).toBe(0);
  expect(i.offset).toBe(660); //+11:00
  expect(i.zoneName).toBe("Australia/Sydney");

  i = DateTime.fromFormat(
    "2021-10-03T02:30:00.000+11:00[Australia/Sydney]",
    "yyyy-MM-dd'T'HH:mm:ss.SSSZZ[z]",
    { setZone: true }
  );
  expect(i.isValid).toBe(true);
  expect(i.year).toBe(2021);
  expect(i.month).toBe(4);
  expect(i.day).toBe(4);
  expect(i.hour).toBe(3);
  expect(i.minute).toBe(30);
  expect(i.second).toBe(0);
  expect(i.millisecond).toBe(0);
  expect(i.offset).toBe(660); //+11:00
  expect(i.zoneName).toBe("Australia/Sydney");
});

I don't know if this should be the behaviour, but I do think it should be defined.

@icambron
Copy link
Member Author

@brendan-gero-humanetix That should be the behavior. Putting aside the +10 in the string for a sec, this is from the docs:

Some local times simply don't exist. The Spring Forward DST shift involves shifting the local time forward by (usually) one hour. In my zone, America/New_York, on March 12, 2017 the millisecond after 1:59:59.999 is 3:00:00.000. Thus the times between 2:00:00.000 and 2:59:59.999, inclusive, don't exist in that zone. But of course, nothing stops a user from constructing a DateTime out of that local time.

If you create such a DateTime from scratch, the missing time will be advanced by an hour:

DateTime.local(2017, 3, 12, 2, 30).toString(); //=> '2017-03-12T03:30:00.000-04:00'

That behavior is covered by other tests.

So back to strings with offsets and zones: because we're ignoring the +10 when it's wrong (and it's hard to argue it isn't wrong), we use the IANA zone to govern the jump forward, per the logic above. So I'm not really sure this is a special case?

@brendan-gero-humanetix
Copy link

@icambron Okay, I think that makes perfect sense then. Will this branch be merged?

@icambron icambron merged commit 3c92310 into master Dec 8, 2021
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.

None yet

2 participants