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

[Merged by Bors] - convert inner datetime to local in to_date_string #1953

Closed

Conversation

superhawk610
Copy link
Contributor

@superhawk610 superhawk610 commented Mar 17, 2022

This Pull Request fixes/closes #1942.

Date.prototype.toDateString should return a value representing the local date. The Rust Date inner value represents UTC time, so it should be adjusted to local time before formatting (see equivalent conversions performed by to_string and to_time_string).

To verify this is working as intended, run the test suite with your OS timezone set to GMT+0, then again with GMT+10. The test date_proto_to_date_string should pass for each. For me (Ubuntu via WSL), this can be done with sudo dpkg-reconfigure tzdata.

This PR also fixes a couple other test cases that used the wrong month value (as noted at the top of the file, JS months are 0-based while chrono months are 1-based).

`Date.prototype.toDateString` should return a value representing the
local date. The Rust `Date` inner value represents UTC time, so it
should be adjusted to local time before formatting (see `to_string` and
`to_time_string`).
@Razican Razican added bug Something isn't working test Issues and PRs related to the tests. builtins PRs and Issues related to builtins/intrinsics labels Mar 17, 2022
@Razican Razican added this to the v0.15.0 milestone Mar 17, 2022
@codecov
Copy link

codecov bot commented Mar 17, 2022

Codecov Report

Merging #1953 (82101ba) into main (aaa07cf) will not change coverage.
The diff coverage is 100.00%.

@@           Coverage Diff           @@
##             main    #1953   +/-   ##
=======================================
  Coverage   45.87%   45.87%           
=======================================
  Files         206      206           
  Lines       17102    17102           
=======================================
  Hits         7846     7846           
  Misses       9256     9256           
Impacted Files Coverage Δ
boa_engine/src/builtins/date/mod.rs 77.50% <100.00%> (ø)

Continue to review full report at Codecov.

Legend - Click here to learn more
Δ = absolute <relative> (impact), ø = not affected, ? = missing data
Powered by Codecov. Last update aaa07cf...82101ba. Read the comment docs.

Copy link
Member

@Razican Razican left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thank you for the contribution! Indeed this makes much more sense :)

Copy link
Member

@HalidOdat HalidOdat left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks good to me!

Copy link
Contributor

@RageKnify RageKnify left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

bors r+
(there seems to be some issues with CI so this will probably take a bit)

@jedel1043
Copy link
Member

bors r+

bors bot pushed a commit that referenced this pull request Mar 17, 2022
This Pull Request fixes/closes #1942.

`Date.prototype.toDateString` should return a value representing the local date. The Rust `Date` inner value represents UTC time, so it should be adjusted to local time before formatting (see equivalent conversions performed by `to_string` and `to_time_string`).

To verify this is working as intended, run the test suite with your OS timezone set to `GMT+0`, then again with `GMT+10`. The test `date_proto_to_date_string` should pass for each. For me (Ubuntu via WSL), this can be done with `sudo dpkg-reconfigure tzdata`.

This PR also fixes a couple other test cases that used the wrong month value (as noted at the top of the file, JS months are 0-based while `chrono` months are 1-based).
@bors
Copy link

bors bot commented Mar 17, 2022

Pull request successfully merged into main.

Build succeeded:

@bors bors bot changed the title convert inner datetime to local in to_date_string [Merged by Bors] - convert inner datetime to local in to_date_string Mar 17, 2022
@bors bors bot closed this Mar 17, 2022
@superhawk610 superhawk610 deleted the fix/date-proto-to-date-string branch March 17, 2022 17:42
Razican pushed a commit that referenced this pull request Jun 8, 2022
This Pull Request fixes/closes #1942.

`Date.prototype.toDateString` should return a value representing the local date. The Rust `Date` inner value represents UTC time, so it should be adjusted to local time before formatting (see equivalent conversions performed by `to_string` and `to_time_string`).

To verify this is working as intended, run the test suite with your OS timezone set to `GMT+0`, then again with `GMT+10`. The test `date_proto_to_date_string` should pass for each. For me (Ubuntu via WSL), this can be done with `sudo dpkg-reconfigure tzdata`.

This PR also fixes a couple other test cases that used the wrong month value (as noted at the top of the file, JS months are 0-based while `chrono` months are 1-based).
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working builtins PRs and Issues related to builtins/intrinsics test Issues and PRs related to the tests.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

date_proto_to_date_string can fail due to timezones
5 participants