Parse ambiguous timezones #450
Conversation
|
Hello @JoakimNil! Thanks for your great work! I think your timezone change is pretty nice and sophisticated. Could you please reply to my code review comments before I merge it? |
|
@wanasit thanks for taking the time to follow up this PR. I can't seem to find your comments. Perhaps they are still pending in an unsubmitted review? |
src/index.ts
Outdated
|
|
||
| export { en, Chrono, Parser, Refiner }; | ||
| export { en, Chrono, Parser, Refiner, TimezoneAbbrMap, ParsingResult }; |
There was a problem hiding this comment.
If we are going to export TimezoneAbbrMap on the API, could we move the interface definition here and make it easier to use (updating documentation would be nice too)
This would be the API/interface, I recommend:
export interface ParsingOption {
...
timezones?: TimezoneMap;
}
...
export type TimezoneMap = {
[key: string]: number | TimezoneOffsetWithDstInfo ;
};
export interface TimezoneOffsetWithDstInfo {
timezoneOffsetDst: number;
timezoneOffsetNonDst: number;
dstStart: (year: number) => Date;
dstEnd: (year: number) => Date;
}
(see my comment about the TimezoneMap and TimezoneOffsetWithDstInfo below)
There was a problem hiding this comment.
Please see the newest changes. The changes are mostly according to your suggestion, and there's now better documentation :)
However, I kept the name AmbiguousTimezoneMap because I feel like it better describes the uncertainty associated with parsing these kinds of timezones. Moreover, I feel like "WithDstInfo" may incorrectly give the impression that these timezones are like normal timezone mappings, but with additional information. On the contrary, the use of these mappings reduces the certainty of the parsing due to the DST ambiguity.
|
@JoakimNil Ops. Sorry I am not really familiar with GitHub review 😓 |
Addresses parsing of timezones like ET where it's unclear if DST should be applied or not. Uses the parsed date (fallback to refDate then current date) to determine whether to apply DST.
Addresses many of @wanasit's comments on PR wanasit#450
JoakimNil
force-pushed
the
ParseAmbiguousTimezones
branch
from
d21b25f to
1b1199c
Compare
|
@wanasit I finally got around to addressing your suggestions. I think the PR is significantly improved after taking them into account. |
|
@JoakimNil Thanks for working on this. I added a few additional comments, but it's a significant improvement as you said. If you don't have time to address the comments, I'll still merge it later this week. |
|
@wanasit sure, I have some spare time. I'm not seeing the new comments though? |
| expect(result.text).toBe(text); | ||
| expect(result.start.isCertain("timezoneOffset")).toBe(true); | ||
| expect(result.start.get("timezoneOffset")).toBe(60); |
There was a problem hiding this comment.
Could you add a new test rather than changing the current one?
similar comments for other changes in this file.
There was a problem hiding this comment.
Without changing the test, some of them will fail after the change. This is because they expect a date during DST with CET timezone to have an offset of 60 minutes, while in fact it should be 120 minutes. I refer to my previous notes (here and in #447) for more details regarding CET offsets.
I also changed a few tests that weren't incorrectly formulated and thus didn't strictly need to be changed. However, they all had really unfortunate reference dates that I think made them hard to understand.
The reference date March 28th happens during DST in some years, and outside of DST in other years. So to understand why these tests should result in offsets of 60 and not 120 requires the developer to be extremely conscious of the nuances of how ambiguous timezones are parsed. Testing ambiguous timezone edge cases is not the purpose of these tests. Therefore, I think it's better to keep those tests in en_timezone_exp.test.ts, and instead change these as I have done to make them easier to interpret for their intended purpose.
|
Ops. Sorry. I forgot to click send the review again. |
…d remove DST transition focus from func docstrings
|
@wanasit I've now addressed your comments with a few new commits. Also see my reply regarding tests, which I didn't address with any code changes. Would you mind taking a look at the new changes? |
|
Cool! thanks again for working on this!. |
Closes #447. I changed my mind, and decided that a PR is probably the best way to focus the discussion 😄
Please see the note in #447 regarding CET. There were some existing tests that expected CET to be 60 minutes, even during DST. I changed those tests to reflect the nuance in interpreting CET. I expect there will be cases where people expect CET to be 60 minutes even during DST, but I firmly believe that it's more common to expect CET to be 120 minutes during DST.
I also made some structural changes: A lot of the timezone logic was duplicated in
timezone.tsandExtractTimezoneAbbrRefiner.ts, so I moved most of it totimezone.ts.Finally, there's a drive-by commit to export ParsingResult, which is commonly needed by chrono users.