✨ Temporal and DST fix #226
Conversation
blakeembrey
left a comment
blakeembrey
left a comment
There was a problem hiding this comment.
This is more a migration to Temporal and doesn't fix DST right now.
wesleytodd
left a comment
wesleytodd
left a comment
There was a problem hiding this comment.
I will let @blakeembrey and @ctcpip sort out the timezone discussion before making sure I fully review the tests, but the rest looks good with just minor nit/questions.
Co-authored-by: Blake Embrey <[email protected]>
|
this PR pulls in Blake's changes from #225, and modified to use |
| const hasOffset = /[+-]\d{2}:?\d{2}/.test(startStr) | ||
| const hasTimezone = /\[[^\]]+\]/.test(startStr) | ||
| const hasUTC = /Z$/.test(startStr) | ||
| let instant, timezone |
There was a problem hiding this comment.
| let instant, timezone | |
| let instant | |
| let timezone |
Sorry for the nit pick, but not only did I think the linter would have complained on this, I will personally. I have two unreasonable opinions as far as formatting goes:
- single variable declarations only: Except when de-structuring, this pattern ends with sloppy code and bad diffs later for zero benefit.
- Always use
{}andreturnin arrow functions: similarly ends with hard to read sloppy code and bad diffs for no benefit.
Terseness when typing is never more important than the time spent later reading the code. I usually wouldn't make these types of comments as they are silly and nitpicking, but I know you and I have some fun discussions when it comes to silly pedantic opinions like this lol, so figured I would. Not blocking, to be clear.
There was a problem hiding this comment.
single variable declarations only
for assignment, yes 100% agree. but if no assignment, then it doesn't matter
Always use {} and return in arrow functions
I tend to agree, but when it's really straightforward, e.g. the .sort(), it's fine to omit
| return next | ||
| } | ||
|
|
||
| const getNextScheduledMeeting = module.exports.getNextScheduledMeeting = function (schedules = [], now = Temporal.Now.instant()) { |
There was a problem hiding this comment.
Does Temporal.Now.instant() include a timezone? I was just wondering since I don't know Temporal well yet.
There was a problem hiding this comment.
UTC
> Temporal.Now.instant()
Temporal.Instant 2025-08-07T15:00:02.637Z
and fixed hackmd by pulling from #226
3 participants
this PR does the following:
Z) is specifiedresolves #212