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 weeksInYear and isoWeeksInYear #1462

Merged
merged 1 commit into from
Feb 4, 2014

Conversation

ichernev
Copy link
Contributor

@ichernev ichernev commented Feb 4, 2014

This implements #1289. I think the week and iso week stuff should go in
a separate calendar at some point.

This implements moment#1289. I think the week and iso week stuff should go in
a separate calendar at some point.
@ichernev ichernev mentioned this pull request Feb 4, 2014
@mdxs
Copy link
Contributor

mdxs commented Feb 4, 2014

The following test could be added to test/moment/weeks_in_year.js to ensure a full coverage of the new isoWeeksInYear function in the current 2000 based cycle of 400 years:

    "count years with 53 iso weeks in year" : function (test) {
        // Based on http://en.wikipedia.org/wiki/ISO_week_date (as seen on 2014-01-06)
        // stating that there are 71 years in a 400-year cycle that have 53 weeks;
        // in this case reflecting the 2000 based cycle
        var count = 0, i, wks, yr;
        for (i = 0; i < 400; i++) {
            yr = 2000 + i;
            wks = (moment.utc([yr, 11, 31]).isoWeek() === 53) ? 53 : 52;
            count += (wks === 53) ? 1 : 0;
            test.equal(moment([yr]).isoWeeksInYear(), wks, yr + " should have " + wks + " iso weeks");
        }
        test.equal(count, 71, "Should have 71 years in 400-year cycle with 53 iso weeks");

        test.done();
    }

Based on one liner implementation for ISO as discussed in #1289 and on Wikipedia page referenced (see comments in above proposed additional test; note that there are similar/related tests in test/moment/weeks.js).

From local testing I can confirm that your implementation passes the above test, so 👍

@ichernev
Copy link
Contributor Author

ichernev commented Feb 4, 2014

This is harder to read and I think the additional tests are more likely to fail in a browser with bad date support. So I'm against. @icambron can decide.

@icambron
Copy link
Member

icambron commented Feb 4, 2014

Yeah, I don't trust browsers to pass that test consistently.

icambron added a commit that referenced this pull request Feb 4, 2014
@changelog
@section feature
@description Add weeksInYear and isoWeeksInYear
@icambron icambron merged commit 5a9e469 into moment:develop Feb 4, 2014
@mdxs
Copy link
Contributor

mdxs commented Feb 4, 2014

No problem.

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.

None yet

3 participants