Build an ISO compliant string - 11507 #12155
Conversation
|
Thank you for the pull request, @Alforoan! Welcome to the Cesium community! In order for us to review your PR, please complete the following steps:
Review Pull Request Guidelines to make sure your PR gets accepted quickly. |
|
Thanks @Alforoan! I can confirm we have a CLA on file from you now. |
|
@jjspace Can you please review? |
There was a problem hiding this comment.
Thanks @Alforoan, it's a good start I just had some comments.
Also can you please add or update the tests for this class to verify the new changes work as expected (and so we know they don't break in the future)? A test that specifically uses the value shown to cause the error in the original issue would be good. It looks like a couple are already failing from the changes so please make sure those are passing again. You can run the tests locally with npm run test, check out the testing guide for more details
| const fractionalSeconds = (second + millisecond * 0.001).toFixed(precision); | ||
| const parts = fractionalSeconds.split('.'); | ||
| millisecondStr = parts.length > 1 ? parts[1] : ''; |
There was a problem hiding this comment.
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?
There was a problem hiding this comment.
I am splitting on . to separate the fractional part (milliseconds) from the whole seconds.
I am not getting scientific notations for very small numbers.
There was a problem hiding this comment.
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.
|
Hi @Alforoan! Is the plan to still move forward with this PR? Do you need any additional help to get this over the finish line? |
Hi yes |
There was a problem hiding this comment.
- Please check the failing tests and make sure any changes you've made allow them to still pass
- Please add new tests in
JulianDateSpec.jsspecifically for these changes and to ensure milliseconds are preserved as outlined in the original issue- You can check our Testing Guide for more info on how to add and run these
- It looks like you skipped running prettier somehow. please run
npm run prettierand also make sure the pre-commit git hook is set up correctly.
| #!/bin/sh | ||
| . "$(dirname "$0")/_/husky.sh" | ||
|
|
There was a problem hiding this comment.
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
| const fractionalSeconds = (second + millisecond * 0.001).toFixed(precision); | ||
| const parts = fractionalSeconds.split('.'); | ||
| millisecondStr = parts.length > 1 ? parts[1] : ''; |
There was a problem hiding this comment.
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.
|
@Alforoan Is this ready for another look? |
|
I'm closing this issue due to inactivity. If you believe this is still an issue, please feel free to re-open. Thanks! |
Description
The function now properly converts the milliseconds into a fractional second with the specified precision, avoiding scientific notation.
Issue
Fixes #11507
Testing plan
I ran the file and made sure it could run without bugs.
Author checklist
CONTRIBUTORS.mdCHANGES.mdwith a short summary of my change