add $environments arguments to ping methods

[johanrosenson]

Status

READY

Description

Adds a second argument to the thenPingHoneybadger and pingHoneybadgerOnSuccess methods that allows you to specify one or more environments when to checkin, it can be omitted to always checkin (current behaviour).

The PR includes tests.

I also added a string typehint for the $id argument, which should be 100% backwards compatible since the Reporter checkin method already does the same typehint.

Examples

$schedule->command(SendEmails::class)
        ->daily()
        ->thenPingHoneybadger('Jiy63Xw', 'production'); // will only checkin on production

$schedule->command(SendEmails::class)
        ->daily()
        ->thenPingHoneybadger('Jiy63Xw', ['production', 'development']); // will only checkin on production and development

[johanrosenson]

I tried adding tests for pingHoneybadgerOnSuccess but that was not possible because all checks with --prefer-lower failed then, maybe this is the reason there was no tests for that method previously?

Anyway, this is because before Laravel 8.0.0 the exitCode was not set at all for closure based schedules, and before 8.6.0 the exitCode was not set properly, so this makes it impossible to write a test for this that doesn't fall with --prefer-lowest, therefor i have removed the tests for now.

But the onSuccess works with earlier versions (as you surely know), as long as it's not a callback as in the tests.

I can add the tests again if you wish, but i think need some guidance then on how you want them done in that case to get them to pass in all checks.
Or we put it of for the future until minimum version of Laravel is 8.6 :)

[shalvah]

You can try including the tests and skipping them on lower versions. Something like:

if (version_compare(app()->version(), '8.6.0', '<')) {
    $this->markTestSkipped("Laravel < 8.6 doesn't set proper return codes.");
    return;
}

[johanrosenson]

Thanks @shalvah , i have made the change in that fashion now, and I also added a bonus test for pingHoneybadgerOnSuccess similar to the original test for thenPingHoneybadger.

I also refactored the checks inside the macros to try and satisfy codeclimate, i think the first version i had was easier to read and the new version didn't make codeclimate happy anyway.
Do you always follow the advice given by codeclimate or only when reasonable? I think the code will be really messy if we try to refactor here if the only intention is to trick it.

[shalvah]

I like!👍

[shalvah]

Do you always follow the advice given by codeclimate or only when reasonable? I think the code will be really messy if we try to refactor here if the only intention is to trick it.

Eh, I agree. I don't believe in coding to the tool also. This works for me. Explicitness > unnecessary refactoring.

[johanrosenson]

@shalvah thanks for the feedback :)