-
Notifications
You must be signed in to change notification settings - Fork 2
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 when() helper to make conditional effects smaller #26
base: main
Are you sure you want to change the base?
Changes from all commits
f73e99b
6ee6290
da11bc2
c1112a4
c8348d0
2d3e8d6
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -3,3 +3,4 @@ composer.lock | |
.php_cs.cache | ||
.phpunit.result.cache | ||
.php-cs-fixer.cache | ||
.vscode |
Original file line number | Diff line number | Diff line change |
---|---|---|
|
@@ -16,18 +16,18 @@ final class Effect implements Feature { | |
/** | ||
* The condition to check. | ||
* | ||
* @var callable | ||
* @var bool|callable | ||
*/ | ||
private $when; | ||
|
||
/** | ||
* Constructor. | ||
* | ||
* @param callable $when The condition to check. | ||
* @param Feature $then The feature to boot if the condition is met. | ||
* @param bool|callable $when The condition to check. | ||
* @param Feature $then The feature to boot if the condition is met. | ||
*/ | ||
public function __construct( | ||
callable $when, | ||
bool|callable $when, | ||
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I had left this as only
For example, this object:
The boolean is being calculated immediately, but it doesn't need to be until we know we're serving a template, so it's somewhat inefficient. In this example, it would be easy for a reader to assume that So, the closure is longer to write, but I think it makes it harder to make a mistake because it won't ever run until the feature is actually booted. There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. I think this can largely be solved with documentation - we can call out a preference for using the callback for the reasons you've identified, but still have boolean values in case there's a simple comparison that we're doing that can be known ahead of time (e.g., comparing against a constant, like |
||
private readonly Feature $then, | ||
) { | ||
$this->when = $when; | ||
|
@@ -37,7 +37,9 @@ public function __construct( | |
* Boot the feature. | ||
*/ | ||
public function boot(): void { | ||
if ( ( $this->when )() === true ) { | ||
if ( is_callable( $this->when ) && ( $this->when )() === true ) { | ||
$this->then->boot(); | ||
} elseif ( is_bool( $this->when ) && true === $this->when ) { | ||
$this->then->boot(); | ||
} | ||
} | ||
|
Original file line number | Diff line number | Diff line change |
---|---|---|
@@ -0,0 +1,56 @@ | ||
<?php | ||
/** | ||
* GroupTest class file. | ||
* | ||
* @package wp-type-extensions | ||
* | ||
* phpcs:disable Squiz.PHP.DisallowMultipleAssignments.Found, WordPress.Security.ValidatedSanitizedInput | ||
*/ | ||
|
||
namespace Alley\WP\Tests\Feature\Features; | ||
|
||
use Mantle\Testkit\Test_Case; | ||
use Alley\WP\Features\Group; | ||
use Alley\WP\Features\Quick_Feature; | ||
use Alley\WP\Types\Feature; | ||
|
||
/** | ||
* Tests for Group class. | ||
*/ | ||
class GroupTest extends Test_Case { | ||
/** | ||
* Test that group can boot a set of features. | ||
*/ | ||
public function test_it_can_boot_a_group_of_features(): void { | ||
$_SERVER['booted_quick_feature'] = false; | ||
$_SERVER['booted_feature'] = false; | ||
$_SERVER['booted_group_of_groups'] = false; | ||
$_SERVER['included_feature'] = false; | ||
|
||
$group = new Group( | ||
new Quick_Feature( fn () => $_SERVER['booted_quick_feature'] = true ), | ||
new class() implements Feature { | ||
/** | ||
* Boot the feature. | ||
*/ | ||
public function boot(): void { | ||
$_SERVER['booted_feature'] = true; | ||
} | ||
}, | ||
new Group( | ||
new Quick_Feature( fn () => $_SERVER['booted_group_of_groups'] = true ), | ||
), | ||
); | ||
|
||
$group->include( | ||
new Quick_Feature( fn () => $_SERVER['included_feature'] = true ), | ||
); | ||
|
||
$group->boot(); | ||
|
||
$this->assertTrue( $_SERVER['booted_feature'] ); | ||
$this->assertTrue( $_SERVER['booted_quick_feature'] ); | ||
$this->assertTrue( $_SERVER['booted_group_of_groups'] ); | ||
$this->assertTrue( $_SERVER['included_feature'] ); | ||
} | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
It might be interesting to note that in the project where
Effect
was developed, it was originally calledWhen
. @efuller and I had a long conversation about it (https://l.alley.dev/48a45b18ff) that eventually resulted in renaming itEffect
(https://l.alley.dev/88aae5417b).IMO, the double "when" shown here is a bit of a red flag that precision is being lost, which can sort of be illustrated by writing the logic compared to the longer way:
"In the plugin, include an effect that when...then..."
"In the plugin, when when...then..."
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I hear what you're saying, @dlh01, but I also see the value in having a shorthand for conditionally including features. Having the shorthand function be named "when" makes it clear what it's doing. The "double when" problem is, imo, due to the fact that the example uses named parameters. With positional args, it's
$plugin->when( $if_condition, $then_condition )
which reads fine to me.