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

Projects
None yet
3 participants
@ichernev
Contributor

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.

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

@ichernev ichernev referenced this pull request Feb 4, 2014

Closed

WeeksInYear? #1289

@mdxs

This comment has been minimized.

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

This comment has been minimized.

Contributor

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

This comment has been minimized.

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

Merge pull request #1462 from ichernev/feature/weeks-in-year
@changelog
@Section feature
@description Add weeksInYear and isoWeeksInYear

@icambron icambron merged commit 5a9e469 into moment:develop Feb 4, 2014

1 check passed

default The Travis CI build passed
Details
@mdxs

This comment has been minimized.

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