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

Complete re-implementation of JulianDate.parseIso8601 #12

Merged
merged 6 commits into from
May 4, 2012
Merged

Conversation

mramato
Copy link
Contributor

@mramato mramato commented May 3, 2012

See the changes for the gory details. This addresses issue #1 and then some.

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.
@mramato
Copy link
Contributor Author

mramato commented May 4, 2012

ffa88cb is the only change you need to look at. It's an amended commit with both a2f7920 and some cleanup. The other two are just merges.

@pjcozzi
Copy link
Contributor

pjcozzi commented May 4, 2012

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;
Copy link
Member

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.

Copy link
Contributor

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.
@mramato
Copy link
Contributor Author

mramato commented May 4, 2012

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.

pjcozzi added a commit that referenced this pull request May 4, 2012
Complete re-implementation of JulianDate.parseIso8601
@pjcozzi pjcozzi merged commit c1c28f5 into master May 4, 2012
oterral pushed a commit to oterral/cesium that referenced this pull request Jun 3, 2016
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants