Increase readability (Date.parse) + fix: Date.parse should be non-enumerable #310
Conversation
|
Please split this up into multiple PRs - this diff is not readable, so I can't easily review it. Also, since |
| var DateShimParse = function parse(string) { | ||
|
|
||
| var match, year, month, day, hour, minute, second, millisecond | ||
| ,isLocalTime, signOffset, hourOffset, minuteOffset, result; |
There was a problem hiding this comment.
comma first is never acceptable here, and please either put every variable on the same line (regardless of line length, which doesn't matter), or put each variable on its own line.
| var DateShimParse = function parse(string) { | ||
|
|
||
| var match, year, month, day, hour, minute, second, millisecond, isLocalTime, signOffset, hourOffset, minuteOffset, result; | ||
| match = isoDateExpression.exec(string); |
There was a problem hiding this comment.
Please do var match here, instead of initializing all the variables at the top of the function - that's not a style we insist on.
|
The Date shims are very complex - this will need to be tested in ES3 and ES5 versions of IE, Safari, Opera, Firefox, Chrome, etc before merging, and definitely will require automated tests to be added. |
|
Due to the fact that automated tests are required the commit should be closed then. But at least it worth noting that original |
|
I'll double check that - in which browsers? In my understanding, the way JS engines work is that a non-enumerable property that is assigned to using the internal |
|
Wow, you are totally correct, thanks! Fix incoming. |
No description provided.