-
Notifications
You must be signed in to change notification settings - Fork 3.5k
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
Complete re-implementation of JulianDate.parseIso8601 #12
Conversation
1. Takes into account all formats defined in the ISO8601 standard, including those not supported by Date.parse. 2. Supports leap seconds and sub-millisecond times. 3. Break out isLeapYear into a seperate helper function. 4. Remove toYearFraction as it has problems and is unused. 5. Write a bunch of new tests. This does NOT add support for more than 4 digit years, as that is a variant of the standard that requires an agreed upon number of digits that we have not settled on yet. This means that we can't yet support times before 1 B.C. or after 9999 AD. This will be added in the future.
1. Takes into account all formats defined in the ISO8601 standard, including those not supported by Date.parse. 2. Supports leap seconds and sub-millisecond times. 3. Break out isLeapYear into a seperate helper function. 4. Remove toYearFraction as it has problems and is unused. 5. Write a bunch of new tests. This does NOT add support for more than 4 digit years, as that is a variant of the standard that requires an agreed upon number of digits that we have not settled on yet. This means that we can't yet support times before 1 B.C. or after 9999 AD. This will be added in the future.
…o ISO8601 Conflicts: Source/Core/JulianDate.js
Does someone with more problem domain knowlege, perhaps @kring, want to review and merge this? My comments are trivial and cosmetic. |
var month = date.getUTCMonth() + 1; // getUTCMonth returns a value 0-11. | ||
var day = date.getUTCDate(); | ||
var year = date.getUTCFullYear(); | ||
|
||
var a = ((month - 14) / 12) | 0; | ||
var b = (year + 4800 + a) | 0; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I think we can get rid of the | 0 on this line, because adding three integers will yield an integer. Hopefully @shunter will chime in if there's some subtlety I'm missing.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
That sounds right. Some of this may have been changes I made. I erred on the site of truncating every time an expression was assigned to something that was an int in the Components code, but I think you're right that adding or subtracting integers stored as doubles always stay integral.
1. Better detect other forms of invalid input. 2. Fix a few cases where valid input is handleded incorrectly. 3. Additional tests for both of the above.
Okay, I think all review comments have been addressed at this point; with the only exception being.. exceptions. I didn't change the code to not throw as we proposed because I'm not completely convinced of how it should be. We should probably have a discussion (maybe on the mailing list) regarding best practices in JavaScript and what works best for our API as a whole. |
Complete re-implementation of JulianDate.parseIso8601
See the changes for the gory details. This addresses issue #1 and then some.