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

Add support for parsing weekdays and week numbers. #764

Closed
wants to merge 1 commit into from
Closed

Add support for parsing weekdays and week numbers. #764

wants to merge 1 commit into from

Conversation

jasondavies
Copy link
Contributor

Also includes support for parsing "%" literals.

var date = new d3_time(0);
if ("j" in d) {
date.setFullYear(d.y, 0, 1);
date.setTime(+date + d.j * 864e5);
Copy link
Member

Choose a reason for hiding this comment

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

Does this work correctly with daylight savings because we later call setHours?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Good point! It turns out this is error-prone for some time zones e.g.:

var d = new Date(0);
d.setFullYear(2012, 0, 1);
d.setTime(+d + 100 * 864e5);
console.log(d);
$ node tz.js 
Tue Apr 10 2012 01:00:00 GMT+0100 (BST)
$ TZ=America/Scoresbysund node tz.js 
Wed Apr 11 2012 00:00:00 GMT+0000 (EGST)

Rather than fiddling with offsets directly, it turns out there's an elegant solution: simply pass the full day-of-year to setFullYear and it will automatically wrap to the correct month and day.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

I've fixed a similar issue for week-of-year/day-of-week parsing too.

@jasondavies
Copy link
Contributor Author

Rebased against version 3.1.6.

@julienfr112
Copy link

can this pull request be merged ?

Also includes support for parsing "%" literals.
@jasondavies
Copy link
Contributor Author

Can we include this in 3.2? It seems like a simple enough change.

@mbostock
Copy link
Member

mbostock commented Jun 8, 2013

Sure, that sounds reasonable. I haven’t closely examined this pull request, but as a skim read, it looks good.

@jasondavies
Copy link
Contributor Author

Excellent, thanks. Staged in #1285 for 3.2.

@jasondavies jasondavies closed this Jun 8, 2013
@jasondavies jasondavies deleted the time-parse-weeks branch June 8, 2013 19:20
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.

None yet

3 participants