PreviousDayOfWeek(7) gives me five days prior.

[sshine]

Palm Sunday is the Sunday before Easter Sunday. Finding Easter Sunday can be achieved with:

use Time::Moment;
use Time::Moment::Adjusters qw(WesternEasterSunday PreviousDayOfWeek);

# 2018-04-01T00:00:00Z
my $easter_sunday = Time::Moment->new(year => 2018)->with(WesternEasterSunday);

But if I apply Time::Moment::Adjusters::PreviousDayOfWeek(7), I get a Tuesday five days earlier!

# 2018-03-27T00:00:00Z
my $palm_sunday = $easter_sunday->with(PreviousDayOfWeek(7));

Since Palm Sunday is always seven days prior to Easter Sunday, I could achieve this with Time::Moment's minus_days(7), but since I want to find a number of other holidays that are much simpler to find using Time::Moment::Adjusters, I would really like to find the root of this unexpected behavior.

Is this a bug?

(Also, I asked on StackOverflow before finding this issue tracker.)

[simbabque]

There seems to be no test coverage for Time::Moment::Adjusters::PreviousDayOfWeek. I wrote an all-pairs test for it, which fails most tests. Only very few of them pass.

use Test::More;

BEGIN {
  use_ok('Time::Moment');
  use_ok('Time::Moment::Adjusters');
  Time::Moment::Adjusters->import('PreviousDayOfWeek');
}

my @name = ( undef, qw/Monday Tuesday Wednesday Thursday Friday Saturday Sunday/ );
my @map  = (
    undef,
    [ undef, 7, 6, 5, 4, 3, 2, 1 ], # n/a, Mon - 1 = Sun, Mon - 2 = Sat, ..., Mon - 7 = Mon
    [ undef, 1, 7, 6, 5, 4, 3, 2 ], # n/a, Tue - 1 = Mon, Tue - 2 = Sun, ..., Tue - 7 = Tue
    [ undef, 2, 1, 7, 6, 5, 4, 3 ], # n/a, Wed - 1 = Tue, Wed - 2 = Mon, ..., Wed - 7 = Wed
    [ undef, 3, 2, 1, 7, 6, 5, 4 ], # n/a, Thu - 1 = Wed, Thu - 2 = Tue, ..., Thu - 7 = Thu
    [ undef, 4, 3, 2, 1, 7, 6, 5 ], # n/a, Fri - 1 = Thu, Fri - 2 = Wed, ..., Fri - 7 = Fri
    [ undef, 5, 4, 3, 2, 1, 7, 6 ], # n/a, Sat - 1 = Fri, Sat - 2 = Thu, ..., Sat - 7 = Sat
    [ undef, 6, 5, 4, 3, 2, 1, 7 ], # n/a, Sun - 1 = Sat, Sun - 2 = Fri, ..., Sun - 7 = Sun  
);

foreach my $day ( 1 .. 7 ) {
    my $tm = Time::Moment->from_string(
        sprintf( '2018-01-%02dT12:30:45.123456789Z', $day )
    );
    foreach my $offset ( 1 .. 7 ) {
        my $got = $tm->with( PreviousDayOfWeek($offset) )->day_of_week;
        is(
            $got,
            $map[$day]->[$offset],
            sprintf( "%s - %d days = %s", $name[$day], $offset, $name[ $map[$day]->[$offset] ] )
        );
    }
}

done_testing;

It might be possible that both @sshine and me misunderstand what this functionality is for, but it seems this is not working.

[sshine]

@simbabque: I'll add your test. I just made one myself to better convince myself.

[chansen]

Thank you for the report and pull request, and thank you @simbabque for the test! I have shipped v0.44 to CPAN.

-- chansen

[simbabque]

Great, thank you so much. I haven't looked at the pr yet, but I'm wondering are the other functions also affected by this bug?

On Wed, May 9, 2018, 20:18 Christian Hansen notifications@github.com wrote:

Thank you for the report and pull request, and thank you @simbabque https://github.com/simbabque for the test! I have shipped v0.44 to CPAN https://metacpan.org/release/CHANSEN/Time-Moment-0.44.

-- chansen

— You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub https://github.com/chansen/p5-time-moment/issues/31#issuecomment-387846493, or mute the thread https://github.com/notifications/unsubscribe-auth/ABEqy-oAywobgQiEM4zzR1DdX3Ehex0Uks5tw0D3gaJpZM4T4Cqt .

[chansen]

I have added tests for NextDayOfWeek(), NextOrSameDayOfWeek(), PreviousDayOfWeek() and PreviousOrSameDayOfWeek() but there could be potential bugs in other Adjuster routines as they are not proven (most routines was ported from a C library and is used in production, there was an typo in the PreviousDayOfWeek() port). Nevertheless they need to be proven, I should start a proper task list for them.

-- chansen

[chansen]

I have started https://github.com/chansen/p5-time-moment/issues/33, if you want to contribute and/or become a co-maintainer let me know! Thanks for your contributions, I'm closing this issue as I think it's resolved.

-- chansen