Int weekday fix by Alex9779 · Pull Request #48 · mysterioustrousers/MTDates

Alex9779 · 2014-10-09T10:01:22Z

New pull request for previously closed one...

…t explicitly by "+ (void)mt_setFirstDayOfWeek:(NSUInteger)firstDay" or implicitly by locale)

yas375 · 2014-10-09T11:47:38Z

MTDatesTests/MTDatesTests.m

Have you tried to run unit tests locally? I don't think they pass to you. At least I can't figure out how unit tests can pass locally to you...

This probably should be XCTAssertEqualObjects. That's why the build on TravisCI has been failed...

Alex9779 · 2014-10-09T11:52:13Z

Yup sorry... Did another commit...

Alex9779 · 2014-10-09T11:57:09Z

Oh man... I use this code in my app but didnt use the test for a while... Have to check later again...

yas375 · 2014-10-09T12:10:02Z

I'm not sure this it actually real problem in the MTDates. I can suppose that the added unit test can give false-positive feedback because you are changing first day of week only in the calendar used by MTDates internally...

The current implementations of mt_weekdayOfWeek: and mt_startOfCurrentWeek: seems to be correct to me... I'm sorry, but I still don't get the reason why it needs this fix...

And even more, which is more important. I've just tried to use the version of MTDates with this fix and I see failing unit tests of my app!!

        context(@"when week starts from Monday", ^{
          it(@"sets correct weekday", ^{
            NSCalendar *calendar = [[NSCalendar alloc] initWithCalendarIdentifier:NSGregorianCalendar];
            calendar.firstWeekday = 2;
            [NSCalendar stub:@selector(currentCalendar) andReturn:calendar];
            [NSDate mt_setFirstDayOfWeek:2]; // Monday

            event.startsAt = [NSDate mt_dateFromYear:2012 month:8 day:7]; // trigger recurrence rule update
            [[event.recurrenceRule[@"weekday"] should] equal:@"tuesday"];

            [NSDate mt_setFirstDayOfWeek:0]; // To reset to default value
          });
        });

@atomkirk this patch doesn't seem to be correct to me.

Alex9779 · 2014-10-09T12:16:20Z

You may be right.
I developed this fix a while ago for iOS 7. Maybe Apple fixed it internally so my fix is not needed anymore.
I will check...

yas375 · 2014-10-09T12:18:23Z

@Alex9779 thank you! The dates are hard))) I'm not 100% sure I'm right, but at least I see a failing test in my production app with the patch. sorry...

Alex9779 · 2014-10-09T12:29:10Z

Np.
ThoughtI could do this quick, currently on some other projects, thought I could do a quick update of my app and then of this pull req, but no :)...
Checking if this is still needed... Will report back...

Alex9779 · 2014-10-09T19:29:59Z

Ok I ran the test now locally. Try running the test I created with and without the patch.
With the patch all tests succeed as you can see the travis build, without my fix the all the tests fail except if start of week is 1=sunday.
The test creates a date and then uses the MTDates function to create another date form the attributes of that date and compares the original date to the one created from the components which should be equal IMHO.
Without the fix and startOfWeek=2 I get a date in return one da earlier than the original up to 6 days earlier when startOfWeek=6.
So for me it seem the fix is still needed because creating a date from its components results in a different date then the original one...

Alex9779 · 2014-10-09T19:33:39Z

Here is my test output without the fix:

Test Case '-[MTDatesTests test_firstDayOfWeek2]' started.
/Users/alexander/Documents/MTDates/MTDatesTests/MTDatesTests.m:1084: error: -[MTDatesTests test_firstDayOfWeek2] : (([NSDate mt_dateFromYear:[date mt_year] week:[date mt_weekOfYear] weekday:[date mt_weekdayOfWeek] hour:[date mt_hourOfDay] minute:[date mt_minuteOfHour]]) equal to (date)) failed: ("1986-07-10 15:23:00 +0000") is not equal to ("1986-07-11 15:23:00 +0000")
/Users/alexander/Documents/MTDates/MTDatesTests/MTDatesTests.m:1091: error: -[MTDatesTests test_firstDayOfWeek2] : (([NSDate mt_dateFromYear:[date mt_year] week:[date mt_weekOfYear] weekday:[date mt_weekdayOfWeek] hour:[date mt_hourOfDay] minute:[date mt_minuteOfHour]]) equal to (date)) failed: ("1986-07-09 15:23:00 +0000") is not equal to ("1986-07-11 15:23:00 +0000")
/Users/alexander/Documents/MTDates/MTDatesTests/MTDatesTests.m:1098: error: -[MTDatesTests test_firstDayOfWeek2] : (([NSDate mt_dateFromYear:[date mt_year] week:[date mt_weekOfYear] weekday:[date mt_weekdayOfWeek] hour:[date mt_hourOfDay] minute:[date mt_minuteOfHour]]) equal to (date)) failed: ("1986-07-15 15:23:00 +0000") is not equal to ("1986-07-11 15:23:00 +0000")
/Users/alexander/Documents/MTDates/MTDatesTests/MTDatesTests.m:1105: error: -[MTDatesTests test_firstDayOfWeek2] : (([NSDate mt_dateFromYear:[date mt_year] week:[date mt_weekOfYear] weekday:[date mt_weekdayOfWeek] hour:[date mt_hourOfDay] minute:[date mt_minuteOfHour]]) equal to (date)) failed: ("1986-07-14 15:23:00 +0000") is not equal to ("1986-07-11 15:23:00 +0000")
/Users/alexander/Documents/MTDates/MTDatesTests/MTDatesTests.m:1112: error: -[MTDatesTests test_firstDayOfWeek2] : (([NSDate mt_dateFromYear:[date mt_year] week:[date mt_weekOfYear] weekday:[date mt_weekdayOfWeek] hour:[date mt_hourOfDay] minute:[date mt_minuteOfHour]]) equal to (date)) failed: ("1986-07-13 15:23:00 +0000") is not equal to ("1986-07-11 15:23:00 +0000")
/Users/alexander/Documents/MTDates/MTDatesTests/MTDatesTests.m:1119: error: -[MTDatesTests test_firstDayOfWeek2] : (([NSDate mt_dateFromYear:[date mt_year] week:[date mt_weekOfYear] weekday:[date mt_weekdayOfWeek] hour:[date mt_hourOfDay] minute:[date mt_minuteOfHour]]) equal to (date)) failed: ("1986-07-05 15:23:00 +0000") is not equal to ("1986-07-11 15:23:00 +0000")
Test Case '-[MTDatesTests test_firstDayOfWeek2]' failed (0.028 seconds).

yas375 · 2014-10-09T21:08:19Z

@Alex9779 looks like I'm starting to understand the issue you are fixing :) However I'm not completely understand why one of the tests in my production code fails with the fix... Maybe I have a bug somewhere? ;) I'll double check it later.

So can we say then that mt_weekdayOfWeek should return the same value in spite of the first weekday of week? If today is Thursday, then mt_weekdayOfWeek should return 5 in any place of the world (with any first day of the week). Does it sounds right to you?

Here is the test explaining this case I'd like to propose to include as well:

- (void)test_weekdayOfWeekRemainsTheSameInSpiteOfFirstDayOfAWeek
{
    NSDate *date = [_formatter dateFromString:@"10/09/2014 05:00am"]; // Thursday

    [NSDate mt_setFirstDayOfWeek:1];
    XCTAssertEqual([date mt_weekdayOfWeek], 5);

    [NSDate mt_setFirstDayOfWeek:2];
    XCTAssertEqual([date mt_weekdayOfWeek], 5);

    [NSDate mt_setFirstDayOfWeek:3];
    XCTAssertEqual([date mt_weekdayOfWeek], 5);

    [NSDate mt_setFirstDayOfWeek:4];
    XCTAssertEqual([date mt_weekdayOfWeek], 5);

    [NSDate mt_setFirstDayOfWeek:5];
    XCTAssertEqual([date mt_weekdayOfWeek], 5);

    [NSDate mt_setFirstDayOfWeek:6];
    XCTAssertEqual([date mt_weekdayOfWeek], 5);

    [NSDate mt_setFirstDayOfWeek:7];
    XCTAssertEqual([date mt_weekdayOfWeek], 5);
}

Does this sounds right to you?

Actually exactly this test shows a regression in mt_weekdayOfWeek because now it can return value more then 7 which is probably not correct...

I've fixed it by adding % 7 before return.

And also seems reasonable to improve test_firstDayOfWeek by asserting actually date objects, and not only strings. And then it turns out that your fix in mt_startOfCurrentWeek is not completely done, but looks like the direction is correct 👍

Give me a few minutes and I'll push what I have :)

Alex9779 · 2014-10-09T21:26:28Z

Now I remember the real problem I had, took a bit but your explanation posted it out to me again. The method you use to GET the weekday always returns the weekday based on 1=sunday no matter what your start of the week is set to. So whatever calendar you use if you wanna GET the weekday of a date it is always right.
But if you use the method to create a date by setting its weekday the weekday number you have to set is based on the start of you week.
So if you set your start to 1=sunday all is fine. If you set it to 2=monday then if you wanna create a date by its weekday out have to use 1=monday.
All what I found when I created the fix about half a year ago was, that the problem is not a problem of MTDates but of the underlying Apple function to create a date from a calendar using date components. See line 208 ff in NSDate+MTDates.m.
IMHO that function is buggy because as I told it respects the start of the week of the calendar and assign the weekday number from 1 to 7 according to that set start, not to the global weekday start 1=sunday...

Alex9779 · 2014-10-09T21:27:52Z

Oh and well, my fix fixed my problem, to be able to calculate a date in the future with a given interval but to be a specific weekday.
Maybe you're right, my fix is not catching every case but maybe you can help out ;)

yas375 · 2014-10-09T21:36:14Z

@Alex9779 you definitely did a great work pointing it out! Looks like it doesn't cover all the cases and maybe event introduces some regressions, but don't worry: I'm almost done with the remaining fixes!! Will open pull request (including your commits) very soon :)

yas375 · 2014-10-09T22:06:26Z

@Alex9779 please take a look at #49. I thought I fixed the problems not covered here, but now I see a lot more failing tests inside of my app. Too tired now and stopping at current point. Would be great if you could review and test my pull review as well. Thanks in advance! Thanks for all your work on this!

Alex9779 · 2014-10-10T07:16:36Z

Closing for #49 to continue what started here...

Alex9779 added 2 commits October 9, 2014 11:43

another test for international weekdays

3b76419

fix for weekday calculation when first weekday is not 1=sunday (if se…

9ba332e

…t explicitly by "+ (void)mt_setFirstDayOfWeek:(NSUInteger)firstDay" or implicitly by locale)

yas375 reviewed Oct 9, 2014
View reviewed changes

updated assert calls

6c7b846

Alex9779 force-pushed the IntWeekdayFix branch from da048ea to 6c7b846 Compare October 9, 2014 12:00

yas375 mentioned this pull request Oct 9, 2014

Weekday fixes #49

Open

Alex9779 closed this Oct 10, 2014

Conversation

Alex9779 commented Oct 9, 2014

Uh oh!

yas375 Oct 9, 2014

Choose a reason for hiding this comment

Uh oh!

Alex9779 commented Oct 9, 2014

Uh oh!

Alex9779 commented Oct 9, 2014

Uh oh!

yas375 commented Oct 9, 2014

Uh oh!

Alex9779 commented Oct 9, 2014

Uh oh!

yas375 commented Oct 9, 2014

Uh oh!

Alex9779 commented Oct 9, 2014

Uh oh!

Alex9779 commented Oct 9, 2014

Uh oh!

Alex9779 commented Oct 9, 2014

Uh oh!

yas375 commented Oct 9, 2014

Uh oh!

Alex9779 commented Oct 9, 2014

Uh oh!

Alex9779 commented Oct 9, 2014

Uh oh!

yas375 commented Oct 9, 2014

Uh oh!

yas375 commented Oct 9, 2014

Uh oh!

Alex9779 commented Oct 10, 2014

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants