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

Modify time zone parsing (%Z) to accept any ISO 8601 compatible time zone format #4

Merged
merged 2 commits into from
Oct 18, 2015

Conversation

Mr0grog
Copy link
Contributor

@Mr0grog Mr0grog commented Oct 16, 2015

This is a port of a pull request from the single-module version of D3 3.x (d3/d3#2480). I hadn’t realized the work of splitting up modules was happening at the time I proposed it. Let me know if this is unwelcome or not helpful.

This includes parsing support for:

  • +-hhmm (the only previously accepted format)
  • +-hh:mm
  • +-hh
  • Z (for UTC)

…and addresses an old issue that myself and several others have run into: d3/d3#2055

…zone format.

This includes:
  * +-hhmm (the only previously accepted format)
  * +-hh:mm
  * +-hh
  * Z (for UTC)

This addresses an old issue from the single module version 3.x: d3/d3#2055
return /^[+-]\d{4}$/.test(string = string.slice(i, i + 5))
? (d.Z = -string, i + 5) // sign differs from getTimezoneOffset!
: -1;
var n = /^(Z)|([+-]\d\d)(?:\:?(\d\d))?/.exec(string.slice(i));
Copy link
Member

Choose a reason for hiding this comment

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

Since the maximum number of possible matching characters here is 6 ("+12:34"), I think we want string.slice(i, i + 6) here for efficiency’s sake.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Ah, good point!

@mbostock
Copy link
Member

Thanks for resubmitting this.

* Shortening the candidate string for faster regex execution.
* Using smarter conditions to do math instead of string concatenation when there are no minutes in the TZ specifier.
@Mr0grog
Copy link
Contributor Author

Mr0grog commented Oct 17, 2015

Made both those changes, so this should perform a little better :)

mbostock added a commit that referenced this pull request Oct 18, 2015
Modify time zone parsing (%Z) to accept any ISO 8601 compatible time zone format
@mbostock mbostock merged commit 1e1a354 into d3:master Oct 18, 2015
@mbostock
Copy link
Member

👍

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Development

Successfully merging this pull request may close these issues.

2 participants