-
Notifications
You must be signed in to change notification settings - Fork 16
Replace Luxon with Temporal #31
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
base: master
Are you sure you want to change the base?
Conversation
Would you be willing to review this pull request? |
@mjradwin I will try to take a look over Yom Tov. However, I think this should wait to be released until Temporal has higher usage. |
Not quite there yet. See https://caniuse.com/temporal |
Thanks for reviewing. If you want to wait for the Temporal spec to be fully ratified (it's currently TC39 Stage 3, and not all the way to Stage 4), I do understand. How would you feel if release my fork on npm as |
@mjradwin You can install with npm directly from your fork
|
Hi @BehindTheMath, we'd love to get this merged into your tree if you're willing to review the PR. I understand the reluctance to release. It's just one data point, but we've been using the We'd rather not wait for |
Hi @BehindTheMath, would you be willing to merge this into your tree? I've got at least one request that would benefit from this update |
This commit adds a new test file `tests/ComplexZmanimCalendar.test.ts` with comprehensive unit tests for the `ComplexZmanimCalendar` class. The tests cover all public methods, ensuring their current behavior is captured and protected against future regressions. The test setup uses a fixed location and date for consistency and comparability with other tests in the suite.
…dar.ts`. While working on this, I also found and fixed a bug in the `isAssurBemlacha` method. The issue was that `Temporal.ZonedDateTime` objects were being compared incorrectly. The new tests are located in `tests/ZmanimCalendar.test.ts`. To ensure their accuracy, I generated the expected values from the current version of your library.
Here's an implementation of #28, which replaces the use of Luxon with Temporal.
The transformation was pretty straightforward. Mostly occurrences of
DateTime
becomeTemporal.ZonedDateTime
, however there are some places where we useTemporal.PlainDate
.The other tricky part is comparisons, which require use of a special static function
Temporal.ZonedDateTime.compare()
instead of just using<
or>
or<=
.I've added an additional unit test for the NOAA calculator with an elevation, which lines up to the Java implementation for a handful of functions down to the second.