Skip to content

AddCommand: use force flag #15

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

Merged
merged 2 commits into from
Feb 27, 2018
Merged

Conversation

Slamdunk
Copy link
Contributor

The force flag is unused in the current implementation.

@BrainMaestro
Copy link
Owner

Hey @Slamdunk, thanks for contributing! Not sure how I missed that before 😅

@@ -57,7 +59,10 @@ protected function execute(InputInterface $input, OutputInterface $output)

file_put_contents($filename, $script);
chmod($filename, 0755);
$output->writeln("Added <info>{$hook}</info> hook");
$output->writeln($fileExists
? "<info>{$hook}</info> overridden"
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would prefer "Added <info>{$hook}</info> hook forcefully". The forceful part can even be emphasized with another color. Probably <fg=red> or something like that

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I would prefer to avoid false positive tests between this and:

public function it_adds_hooks_that_do_not_already_exist()
{
$this->commandTester->execute([]);
foreach (array_keys(self::$hooks) as $hook) {
$this->assertContains("Added {$hook} hook", $this->commandTester->getDisplay());
}
}

Because the asserted string would be a subpart of the other.

Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

True. Okay, I would still like to maintain the format, so maybe we can use Overwrote instead of Added. I don't like the verb though.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't like the verb though.

LoL, why? :)

@BrainMaestro BrainMaestro merged commit 2e4d8bd into BrainMaestro:master Feb 27, 2018
@BrainMaestro
Copy link
Owner

@Slamdunk Thanks!

@Slamdunk Slamdunk deleted the add_force_fix branch February 27, 2018 10:24
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants