reset week counter when crossing the new year. #3148
Conversation
Currently January counts weeks as 52, 53, 54, 55, 56 but should count 52, 1, 2, 3, 4. This simple fix should make the counting correct.
Travis build is failing due to formatting error spotted by jshint:
regards |
I've got it passing JSHint now. |
This looks good to me and I don't see any issues popping out of it. 👍 |
I fixed this on a client project by doing:
but really just wanted to say that it's pretty awesome to come here and see a pull request already waiting :) |
Actually... this pull request doesn't work correctly, nor does the line I pasted above. I've had to do this instead:
because weekNumber is initially set to 52, the if condition isn't checked until it's bumped to 53. My modulo version is flawed because the first week on the calendar is really both 52 and 1. |
Recomendation from comments.
sean - I'm sorry, I really don't understand how week counting works. I guess the week starting with January 4th is actually week 1 after all? |
I admit it took me some thinking about. But I thought it odd that 52 and 1 were the same. According to this: http://www.epochconverter.com/date-and-time/weeknumbers-by-year.php?year=2014 The first week of the year apparently did begin December 29th. Though confusingly they count 2015 as having 53 weeks. I guess it has to do with the number of Monday's in a year. I might revert my last commit unless I can think of a better way to match the counting to the calendar at epochconverter.com |
PR needs some tests. |
Closing this as #2306 contains tests, fixes the same issue, and is simpler. |
Currently January counts weeks as 52, 53, 54, 55, 56 but should count 52, 1, 2, 3, 4. This simple fix should make the counting correct.