-
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
Clear unregistered features #59
base: master
Are you sure you want to change the base?
Conversation
/** | ||
* @return string[] | ||
*/ | ||
public function listFeatures(): array; | ||
|
||
public function removeFeature(string $feature): void; |
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.
Since these methods are duplicated in all ConfigurationRepository
interfaces, I think we should introduce a new shared interface that is not about "configuration" but more about "administration" (or a better name if you find one).
$configuredFeatures = array_unique(array_reduce( | ||
$this->strategies, | ||
fn (array $features, TogglingStrategy $strategy) => \array_merge($features, $strategy->listFeatures()), | ||
[] | ||
)); |
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 think we can avoid some array_merge()
calls by doing the following:
$configuredFeatures = array_unique(array_merge(...array_map(
fn (TogglingStrategy $strategy) => $strategy->listFeatures(),
$this->strategies
)));
|
||
$unregisteredFeatures = array_values(array_diff($configuredFeatures, $registeredFeatures)); | ||
|
||
sort($unregisteredFeatures); |
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.
Not sure if this should be part of this API or in the clients of this API: it feels like sorting things should be dealt with at display time...
public function clearFeatureConfiguration(string $feature): void | ||
{ | ||
foreach ($this->strategies as $strategy) { | ||
$strategy->clearFeatureConfiguration($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.
To avoid data loss, I think we should add an assertion to make sure the feature is not registered before removing its configuration.
static::assertSame(['unregistered'], $router->listUnregisteredFeatures()); | ||
} | ||
|
||
public function testConfigurationCanBeClearedForRegisteredFeatures(): void |
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.
Are we sure we want this behaviour?
Like I said in my previous comment, users could lose configuration for registered features.
$count = count($features); | ||
|
||
$io->note(sprintf( | ||
'%d unregistered feature(s) have been found: %s.', |
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.
Nitpick: we could handle pluralisation by using the $count
variable.
closes #46
This PR adds a console command to clear configuration for all unregistered features: