Week formatter fix in date filter (Comply to ISO8601)

[yannickadam]

ISO8601 mandates that a week starts on a Monday, and ends on a Sunday.

The test case was on a Friday, and did not show that W3 was returned instead of W2.
http://plnkr.co/edit/897MdGDME67aIQmLpzqZ

[googlebot]

CLAs look good, thanks!

[pkozlowski-opensource]

@yannickadam thnx for the PR. Could you please squash 2 commits into one and change the commit message to follow our conventions (https://github.com/angular/angular.js/blob/master/CONTRIBUTING.md#-git-commit-guidelines)? Thnx

[yannickadam]

@pkozlowski-opensource Sorry - squashed & updated commit msg.

[pkozlowski-opensource]

Looking into it more carefully it seems like there are more corner cases, especially around "week zero" - specifically I'm not sure that the week no 0 is a valid value in ISO8601 so we definitively need more tests around those corner cases.

If I'm not mistaken, all those tests should be passing:

    it('should format week number correctly (as per ISO_8601)', function () {
      expect(date(new angular.mock.TzDate(+5, '2012-01-01T12:00:00.000Z'), 'EEE (ww)')).toEqual('Sun (01)');
      expect(date(new angular.mock.TzDate(+5, '2012-01-07T12:00:00.000Z'), 'EEE (ww)')).toEqual('Sat (01)');
      expect(date(new angular.mock.TzDate(+5, '2012-01-03T12:00:00.000Z'), 'EEE (ww)')).toEqual('Tue (01)');

      expect(date(new angular.mock.TzDate(+5, '2013-01-11T12:00:00.000Z'), 'EEE (ww)')).toEqual('Fri (02)');
      expect(date(new angular.mock.TzDate(+5, '2013-01-13T12:00:00.000Z'), 'EEE (ww)')).toEqual('Sun (02)');
    });

On top of this I think that the current doc is wrong as it suggest that a week number can be zero:

'ww': ISO-8601 week of year (00-53)

while I don't think it is true.

All in all I think that we need to fix those issues but this PR requires more work.

[pkozlowski-opensource]

One last remark - while the problem raised here is valid fixing it might be seen as a breaking change, especially in countries where Sun is traditionally counted as a first of a week.

[yannickadam]

Indeed, many countries use Sunday as the starting day of the week. Taken from this SO answer: http://stackoverflow.com/questions/727471/how-do-i-get-the-first-day-of-the-week-for-the-current-locale-php-l8n

(001 is "The World")

<firstDay day="mon"  territories="001" />
<firstDay day="fri"  territories="MV" />
<firstDay day="sat"  territories="AF BH DJ DZ EG ER ET IQ IR JO KE KW LY MA OM QA SA SD SO TN YE" />
<firstDay day="sun"  territories="AS AZ BW CA CN FO GE GL GU HK IE IL IN IS JM JP KG KR LA MH MN MO MP MT NZ PH PK SG SY TH TT TW UM US UZ VI ZW" />
<firstDay day="sun"  territories="ET MW NG TJ" draft="unconfirmed" />
<firstDay day="sun"  territories="GB" draft="unconfirmed"  alt="variant" references="Shorter Oxford Dictionary (5th edition, 2002)"/>

I've looked at the Java doc for the Gregorian Calendar, and here is an extract:

The getFirstDayOfWeek() and getMinimalDaysInFirstWeek() values are initialized using locale-dependent resources when constructing a GregorianCalendar. 
The week determination is compatible with the ISO 8601 standard when getFirstDayOfWeek() is MONDAY and getMinimalDaysInFirstWeek() is 4, 
which values are used in locales where the standard is preferred.

So a solution can be built to handle any starting day of the week (even though it is not described in ISO8601).

About the test cases, I agree that more should be written. Week 0 does not truly exists. It is Week 52/53 from the previous year. In your first test, Sun 01 2012 is contained in Week 52 Year 2011. To manage this issue, PHP provides a ISO8601 year:

To recap the todo list:

• Handle any starting day for a week.
• Return the proper week 52/53 instead of 0
• Add the "o" formatter as in PHP for ISO8601 year number
• Test edge cases (First/last days of a year, several days in the week, ...)

Questions:

• $locale does not provide the starting day of the week, nor the minimum number of days to consider the first week of the year. Should Sunday be kept as default to prevent a breaking change?
• How to pass this information to the date filter? A third (optional) argument after the timezone would work, but would make the markup "heavy".

Coming back to this pull-request, I believe it does "fix" the current behavior according to Angular's doc/specification. The work that needs to be done to make it perfect falls in the "feature" category :)

[pkozlowski-opensource]

@yannickadam so I took some time to look into the existing implementation and the ISO standard for week numbers and I think that the current implementation deviates from the ISO standard in 2 ways:

• assuming Sun as the first day of the week
• allowing weeks no 0 for years where the 1st of Jan is Fri, Sat or Sun - my understanding is that week zero doesn't exist in ISO - it would be week 52 / 53 of the previous year.

So I think that the current implementation shouldn't be called ISO-compliant - it simply doesn't follow the standard... And frankly, looking at the ISO spec that mandates turning zero-week into a 52/53 week of a previous year ... I'm not sure this is a standard worth following. I think it might confuse people even more.

Now, given all this, here is my proposal:

• let's keep existing implementation and add tests for all the corner cases
• let's modify the documentation to remove references to ISO-8601 for week numbers - let's document the current behaviour where week 1 is taken as the one that has the first Thursday of a year
• let's work towards making first day of the week configurable - this would be a feature.

How does it sound? I'm going to send a PR with tests that document the current behaviour shortly.