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.

Sign up for GitHub

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

Build an ISO compliant string - 11507 #12155

Closed
wants to merge 8 commits into from
Closed

Build an ISO compliant string - 11507 #12155

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
8 commits
Select commit Hold shift + click to select a range
48bbf8b
add a fix so that the output is iso compliant
Alforoan Aug 26, 2024
36fdc6c
update changes.md and contributors.md
Alforoan Aug 26, 2024
893dc6f
Merge branch 'main' into jay_issue_11507
Alforoan Aug 26, 2024
5314819
Merge branch 'main' into jay_issue_11507
Alforoan Sep 20, 2024
e89af80
change line in changes.md
Alforoan Sep 20, 2024
30bacc8
add a contributor back
Alforoan Sep 23, 2024
6ac8ad8
remove default precision
Alforoan Sep 23, 2024
5bdb97d
moved descrption to under the latest release
Alforoan Sep 23, 2024
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
3 changes: 0 additions & 3 deletions .husky/pre-commit
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,4 +1 @@
#!/bin/sh
. "$(dirname "$0")/_/husky.sh"

Comment on lines -1 to -3
Copy link
Contributor

Choose a reason for hiding this comment

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

This change needs to happen eventually but it should be it's own PR so it's isolated and we can be sure it's the change we want. This PR should focus only on the date string
I had already opened #12180

npx lint-staged
1 change: 1 addition & 0 deletions CHANGES.md
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -1,6 +1,7 @@
# Change Log

### 1.122 - 2024-10-01
- Fix JulianDate to always generate valid ISO strings for fractional milliseconds [#11507](https://github.com/CesiumGS/cesium/issues/11507)

#### @cesium/engine

Expand Down
1 change: 1 addition & 0 deletions CONTRIBUTORS.md
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -408,6 +408,7 @@ See [CONTRIBUTING.md](CONTRIBUTING.md) for details on how to contribute to Cesiu
- [Vladislav Yunev](https://github.com/YunVlad)
- [Levi Montgomery](https://github.com/Levi-Montgomery)
- [Brandon Berisford](https://github.com/BeyondBelief96)
- [Jay Yong](https://github.com/Alforoan)
- [Lawrence Owen](https://github.com/ljowen)
- [Adam Wirth](https://github.com/adamwirth)
jjspace marked this conversation as resolved.
Show resolved Hide resolved
- [Javier Sanchez](https://github.com/jvrjsanchez)
Expand Down
47 changes: 10 additions & 37 deletions packages/engine/Source/Core/JulianDate.js
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -783,40 +783,13 @@ JulianDate.toIso8601 = function (julianDate, precision) {

let millisecondStr;

if (!defined(precision) && millisecond !== 0) {
//Forces milliseconds into a number with at least 3 digits to whatever the default toString() precision is.
millisecondStr = (millisecond * 0.01).toString().replace(".", "");
return `${year.toString().padStart(4, "0")}-${month
.toString()
.padStart(2, "0")}-${day
.toString()
.padStart(2, "0")}T${hour
.toString()
.padStart(2, "0")}:${minute
.toString()
.padStart(2, "0")}:${second
.toString()
.padStart(2, "0")}.${millisecondStr}Z`;
}

//Precision is either 0 or milliseconds is 0 with undefined precision, in either case, leave off milliseconds entirely
if (!defined(precision) || precision === 0) {
return `${year.toString().padStart(4, "0")}-${month
.toString()
.padStart(2, "0")}-${day
.toString()
.padStart(2, "0")}T${hour
.toString()
.padStart(2, "0")}:${minute
.toString()
.padStart(2, "0")}:${second.toString().padStart(2, "0")}Z`;
}

//Forces milliseconds into a number with at least 3 digits to whatever the specified precision is.
millisecondStr = (millisecond * 0.01)
.toFixed(precision)
.replace(".", "")
.slice(0, precision);
if (millisecond !== 0 || defined(precision)) {
// Forces milliseconds into a number with the specified precision, without scientific notation
const fractionalSeconds = (second + millisecond * 0.001).toFixed(precision);
const parts = fractionalSeconds.split('.');
millisecondStr = parts.length > 1 ? parts[1] : '';
Comment on lines +788 to +790
Copy link
Contributor

Choose a reason for hiding this comment

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

Can you expand on why you're adding the seconds here then splitting on the .? I think there may potentially be other, better, ways to get the decimal part of a number if that's the desire.

Also the original issue called out that toFixed does not necessarily eliminate the issue of getting scientific notation when converting a number to string if the precision is right (see the docs). Does this addition avoid that somehow? or is that not accounted for?

Copy link
Author

Choose a reason for hiding this comment

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

I am splitting on . to separate the fractional part (milliseconds) from the whole seconds.
I am not getting scientific notations for very small numbers.

Copy link
Contributor

Choose a reason for hiding this comment

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

My initial thought is that splitting on . is a bad way to manipulate numbers but looking into it a bit this seems like a fine way to do it. However, I'm still unclear about why you're adding the seconds to it first if you just strip them off.
It's possible there's a more simple solution that does include combining the seconds and milliseconds that looks something like this (n * 0.001 + 45).toFixed(precision).padStart(2 + precision + 1, '0'). Just pay special attention to the padding for the seconds.

Also it looks like we have constants for some of these "magic numbers", please swap out 0.001 with TimeConstants.SECONDS_PER_MILLISECOND as we use elsewhere in this code.

}

return `${year.toString().padStart(4, "0")}-${month
.toString()
.padStart(2, "0")}-${day
Expand All @@ -825,9 +798,9 @@ JulianDate.toIso8601 = function (julianDate, precision) {
.toString()
.padStart(2, "0")}:${minute
.toString()
.padStart(2, "0")}:${second
.toString()
.padStart(2, "0")}.${millisecondStr}Z`;
.padStart(2, "0")}:${Math.floor(second).toString().padStart(2, "0")}${
millisecondStr ? `.${millisecondStr}` : ''
}Z`;
};

/**
Expand Down