Implement toZonedDateTime in PlainDate by JohannesHelleve · Pull Request #192 · boa-dev/temporal

JohannesHelleve · 2025-02-12T10:46:26Z

Related to #148

First time contributer, not sure if is correct. Tought this might be a good place to get some feedback while showcasing the code.

Struggling to choose type for the item and how to check if it is an object.

Thanks for the feedback!

Link to method

In kahoots with @brageh01

jedel1043 · 2025-02-12T17:20:13Z

Thank you for contributing!

Yeah, the most difficult part about this is that we can't do a 1-to-1 implementation of the spec because temporal_rs fits between JS engines and the ICU APIs used for timezone and calendar calculations, so certain things like object handling and property fetching are delegated to the engines to do.

As a good rule of thumb, every time you see something like "if o is an object" or "Get(o, 'property')", assume those calculations are made by the engine, and try to receive a struct or an enum as a parameter instead.

jedel1043 · 2025-02-12T17:25:19Z

In this case, you should just take an Option<PlainTime> and a Timezone as parameters.

nekevss · 2025-02-12T17:35:28Z

Agree with the comments above.

For further reference to a similar method implementation wise if you'd like to see how this was handled elsewhere in the library, feel free to check out ZonedDateTime.prototype.withPlainTime in temporal_rs and in Boa.

JohannesHelleve · 2025-02-21T10:57:04Z

Updated the code now. Think im closer to a somewhat solution now. Would appricate some feedback! @jedel1043 @nekevss . Thanks!

nekevss

This is a good start so far! I've left a few comments on points that could be improved.

My general recommendation is try and follow steps by coupling the code together with the commented steps (see duration for an example). I believe most of the internals used in these steps do exist in the codebase, so using the specification steps as a guide can be incredibly useful.

This method is a bit tricky as steps 1-4 require engine specific details, but we can guarantee they are completed by the types of the function args, so the real starting point, as already identified, is step 5.

src/utils.rs

src/builtins/core/date.rs

…an up

JohannesHelleve · 2025-02-26T09:54:08Z

Updated now!

Little bit hard to test since the api needs to work correctly. I think?

nekevss

Looking closer, I left a handful of things I noticed when looking.

When working with rust, it's useful to use the compiler and cargo to help you.

I'd recommend running:

cargo fmt -all, cargo clippy --no-default-features, and cargo clippy --all-features --all-targets

The compiler errors and lints can help massively when working on some code. Also, we use those commands in our CI, so it is needed to pass CI too.

nekevss · 2025-02-26T15:15:25Z

src/builtins/core/date.rs

+        };
+
+        //  7. Return ! CreateTemporalZonedDateTime(epochNs, timeZone, temporalDate.[[Calendar]]).
+        Self::try_new(epoch_ns.0, self.calendar.clone(), tz)


issue: missing bracket to close function

nekevss · 2025-02-26T15:16:43Z

src/builtins/compiled/date.rs

+impl Date {
+
+    /// Converts a `Date` to a `ZonedDateTime` in the UTC time zone.
+    pub fn to_zoned_date_time (self, time_zone: &str) -> TemporalResult<crate::ZonedDateTime> {


issue: function signature here should be the same as to_zoned_date_time_with_provider.

Also, it looks like there is a space after the function name. Although, I'm not entirely sure whether that may affect anything.

nekevss · 2025-02-26T15:17:53Z

src/builtins/compiled/date.rs

+        let provider = TZ_PROVIDER
+            .lock()
+            .map_err(|_| TemporalError::general("Unable to acquire lock"))?;
+        self.to_zoned_date_time_with_provider(time_zone, &*provider)


issue: to_zoned_date_time_with_provider is called with the wrong args.

self.to_zoned_date_time_with_provider(time_zone, plain_time, &*provider)

nekevss · 2025-02-26T15:19:28Z

src/builtins/core/date.rs

+        //     c. If ISODateTimeWithinLimits(isoDateTime) is false, throw a RangeError exception.        
+        //     d. Let epochNs be ? GetEpochNanosecondsFor(timeZone, isoDateTime, compatible).
+        let epoch_ns = if let Some(time) = plain_time {    
+            let temporal_time = Time::new_unchecked(time)?;


issue: I'm not entirely sure what the time is referring to here. But it should be unneeded.

You should be able to access PlainTime's iso field by using time.iso in line 685.

nekevss · 2025-02-26T15:19:50Z

src/builtins/compiled/date.rs

@@ -0,0 +1,12 @@
+use crate::{builtins::TZ_PROVIDER, TemporalError, TemporalResult, Date};
+
+impl Date {


issue: wrong struct name on the impl block -> PlainDate

JohannesHelleve · 2025-02-27T13:02:40Z

Thanks that helped a lot!!

Now stuck on this thing:

Trying to debug, but i can't seem to either use dbg! or println! without importing crates. Others managed without importing.

nekevss · 2025-02-27T15:29:15Z

dbg! or println! without importing crates

Ah, currently temporal_rs is setup for #![no_std], which can be seen here, so std functions and methods are feature flagged.

Due the current default sys feature flag, you should be able to use the full path std::println! vs. println!. The other option is to comment out the #![no_std] in lib.rs when you are debugging, but be careful about not committing the commented out line to the git history (although, it's happened to us all)

JohannesHelleve · 2025-03-06T13:23:17Z

The code compilies now! But all the test fails :(.

My expirence with rust is slim to zero, so i was wondering what the best practice is to debug the code. For now i've tried to insert code in the functions, but it don't get printed in the terminal. I want to see the value of a variable at a certain point.

I've heard with some others and they decided to create their own test. Is this the optimal way?

Thanks again!

jedel1043 · 2025-03-06T16:21:06Z

My expirence with rust is slim to zero, so i was wondering what the best practice is to debug the code. For now i've tried to insert code in the functions, but it don't get printed in the terminal. I want to see the value of a variable at a certain point.

If you use rust-analyzer on VSCode, you can pretty easily hit the "debug" label on any test and it should put you on a debugger environment.
If you use a Jetbrains' IDE (possibly RustRover IIRC), it should have equivalent functionality.
Any other IDE should support lldb or an equivalent to debug a binary program.

JohannesHelleve · 2025-03-24T17:35:14Z

Added some tests now! And all of them passed, but not sure if this is correct.

Thanks for the feedback!

nekevss

One nit related to the lint failure.

Also for cargo fmt failure, sometimes you need to run cargo fmt --all to format the tests. You probably ran cargo fmt, which formatted the code but missed the tests. So running cargo fmt --all should get the tests. 😄

nekevss · 2025-03-24T18:45:24Z

src/builtins/core/date.rs

 }

-#[cfg(test)]
+#[cfg(all(test, feature = "tzdb"))]


nit: the feature configuration shouldn't be here I think. You should be able to configure specific tests. With #[cfg(feature = "tzdb"] above the specific tests. I think references can be found in other test modules.

…temporal into impl-to-zoned-date-time

JohannesHelleve · 2025-03-26T10:44:04Z

Try to run the command, but didn't get any errors.

Anyhow, updated it now. However there seems to be an error on line 82 for me, which comes from another merge.

nekevss · 2025-03-26T13:36:04Z

cargo fmt --all will automatically format the code for you. The CI lint runs a check on that command.

So run cargo fmt --all, commit the changed/reformatted code, and then you should be good to go! 😄

nekevss

LGTM! This should be good to go once the formatting is fixed.

Thanks for working on this!

JohannesHelleve · 2025-04-07T11:14:22Z

Nice!

Tried running the command now. Hope it worked!

nekevss · 2025-04-07T15:14:12Z

Looks great! Thanks for putting all the work into this! 😄

@brageh01

Related to boa-dev#148 First time contributer, not sure if is correct. Tought this might be a good place to get some feedback while showcasing the code. Struggling to choose type for the item and how to check if it is an object. Thanks for the feedback! [Link to method ](https://tc39.es/proposal-temporal/#sec-temporal.plaindate.prototype.tozoneddatetime) In kahoots with @brageh01

JohannesHelleve added 2 commits February 12, 2025 11:32

Not sure check is an object is correct and type of item

3fd2fa3

clean up

ba8c8ab

JohannesHelleve changed the title ~~Impl to zoned date time~~ Implement toZonedDateTime in PlainDate Feb 12, 2025

nekevss requested review from jedel1043 and nekevss February 13, 2025 22:02

JohannesHelleve and others added 3 commits February 20, 2025 21:33

added match stmt

48fcbb6

cleaner code

f769e2a

Merge branch 'main' into impl-to-zoned-date-time

d1556af

nekevss requested changes Feb 21, 2025

View reviewed changes

src/utils.rs Outdated Show resolved Hide resolved

src/builtins/core/date.rs Outdated Show resolved Hide resolved

src/builtins/core/date.rs Outdated Show resolved Hide resolved

src/builtins/core/date.rs Outdated Show resolved Hide resolved

jedel1043 added C-enhancement New feature or request C-api Changes related to the public API labels Feb 24, 2025

Utilized already existing fn, created api, some variable change & cle…

4d5fbca

…an up

Merge branch 'main' into impl-to-zoned-date-time

9c1752a

nekevss requested changes Feb 26, 2025

View reviewed changes

updated with some tips, getting closer

9a80a4d

JohannesHelleve added 3 commits March 6, 2025 14:13

fetched

e5e4e60

merged

03a5bdd

added back wrongly merged code

4147b16

nekevss mentioned this pull request Mar 18, 2025

Implement ToZonedDateTime method for PlainDate #148

Closed

created tests

292b00a

nekevss reviewed Mar 24, 2025

View reviewed changes

JohannesHelleve and others added 3 commits March 25, 2025 17:09

Merge branch 'main' into impl-to-zoned-date-time

deb7cc1

feature flag only for own test

49072bf

Merge branch 'impl-to-zoned-date-time' of github.com:JohannesHelleve/…

cb6550c

…temporal into impl-to-zoned-date-time

nekevss approved these changes Mar 27, 2025

View reviewed changes

JohannesHelleve and others added 2 commits April 7, 2025 13:09

Merge branch 'main' into impl-to-zoned-date-time

4c2ce7b

running lint command

a9b0747

nekevss merged commit ac07ef9 into boa-dev:main Apr 7, 2025
8 checks passed

		@@ -0,0 +1,12 @@
		use crate::{builtins::TZ_PROVIDER, TemporalError, TemporalResult, Date};

		impl Date {

Conversation

JohannesHelleve commented Feb 12, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jedel1043 commented Feb 12, 2025

Uh oh!

jedel1043 commented Feb 12, 2025

Uh oh!

nekevss commented Feb 12, 2025

Uh oh!

JohannesHelleve commented Feb 21, 2025

Uh oh!

nekevss left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

JohannesHelleve commented Feb 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nekevss left a comment

Choose a reason for hiding this comment

Uh oh!

nekevss Feb 26, 2025

Choose a reason for hiding this comment

Uh oh!

nekevss Feb 26, 2025

Choose a reason for hiding this comment

Uh oh!

nekevss Feb 26, 2025

Choose a reason for hiding this comment

Uh oh!

nekevss Feb 26, 2025

Choose a reason for hiding this comment

Uh oh!

nekevss Feb 26, 2025

Choose a reason for hiding this comment

Uh oh!

JohannesHelleve commented Feb 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nekevss commented Feb 27, 2025

Uh oh!

JohannesHelleve commented Mar 6, 2025

Uh oh!

jedel1043 commented Mar 6, 2025

Uh oh!

JohannesHelleve commented Mar 24, 2025

Uh oh!

nekevss left a comment • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

nekevss Mar 24, 2025

Choose a reason for hiding this comment

Uh oh!

JohannesHelleve commented Mar 26, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

nekevss commented Mar 26, 2025

Uh oh!

nekevss left a comment

Choose a reason for hiding this comment

Uh oh!

JohannesHelleve commented Apr 7, 2025

Uh oh!

nekevss commented Apr 7, 2025

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

JohannesHelleve commented Feb 12, 2025 •

edited

Loading

nekevss left a comment •

edited

Loading

JohannesHelleve commented Feb 26, 2025 •

edited

Loading

JohannesHelleve commented Feb 27, 2025 •

edited

Loading

nekevss left a comment •

edited

Loading

JohannesHelleve commented Mar 26, 2025 •

edited

Loading