Rework InfectionApplication class and remove 'application' service #146
Conversation
There was a problem hiding this comment.
Thank you @sidz
Seems like there is no line break anymore between some progress reporters. Could you please check?
Besides that, LGTM.
src/Console/Application.php
Outdated
| new Command\InfectionCommand(), | ||
| ]); | ||
|
|
||
| if ('phar:' === substr(__FILE__, 0, 5)) { |
There was a problem hiding this comment.
noticed report from EA Extended plugin:
0 === strpos(__FILE__, 'phar:')
Use strpos instead of substr to check the start of a string. Substr invokes additional memory allocation, which is not needed in the context. Strpos will do the same job, but without any overhead (it's just searching a string)
|
Done. Also I've seen this issue with it's not happened each time when you start infection. p.s. It might be caused by XdebugHandler |
| use Symfony\Component\Console\Output\OutputInterface; | ||
|
|
||
| /** | ||
| * @method Application getApplication() |
There was a problem hiding this comment.
phpstorm autocomplete. As getApplication by default will return Symfony's Application class which doesn't have getContainer method
| public function getContainer(): Container | ||
| { | ||
| if ($this->container === null) { | ||
| $this->container = new Container($this->getApplication()->getContainer()); |
There was a problem hiding this comment.
$this->getApplication()->getContainer() should be enough here no need to instantiate the container of a container
There was a problem hiding this comment.
Pimple default container doesn't have get/set methods so this wrapper: https://github.com/silexphp/Pimple/blob/master/src/Pimple/Psr11/Container.php give us these methods.
also this wrapper protects our container and doesn't give ability to modify it. I think this is a big plus.
There was a problem hiding this comment.
but if we will inject all dependencies through the constructor we don't need to have this wrapper.
@borNfreee thoughts?
There was a problem hiding this comment.
as far as I understand, to inject dependency to constructor we still need to do new App($container->get('dependency'), right?
If this is a case, let's have psr11 interface. Who knows, maybe one day we will replace pimple with something else
There was a problem hiding this comment.
yeah it will look like this
$commands = array_merge(parent::getDefaultCommands(), [
new Command\ConfigureCommand(),
new Command\InfectionCommand(
$this->container['dep1'],
$this->container['dep1'],
$this->container['dep1'],
......
),
]);
There was a problem hiding this comment.
let's keep psr11 ($this->container->get('dep1'))
There was a problem hiding this comment.
meh, indeed not much better, I thought pimple would resolve those deps I see
| * @var ConsoleHelper | ||
| */ | ||
| private $consoleHelper; | ||
| protected function configure() |
There was a problem hiding this comment.
missing the inherit doc bloc and for the others as well; unless you don't ever want to put them?
There was a problem hiding this comment.
haven't ever had a plan to add anything there and not sure what we need to add :)
There was a problem hiding this comment.
I mean to add @inherit doc blocks when the method has a parent one; but nvm if you don't like that practice. I kinda like it because you can easily tell if it has a parent one without comparing the parent class but that's it
src/Command/InfectionCommand.php
Outdated
| $this->buildDynamicDependencies($input); | ||
| try { | ||
| /** @var EventDispatcher $eventDispatcher */ | ||
| $eventDispatcher = $this->getContainer()->get('dispatcher'); |
There was a problem hiding this comment.
instead of relying on a service locator, couldn't we inject those dependencies through the constructor?
There was a problem hiding this comment.
yeah we can do it manually from our new Application class
src/Command/InfectionCommand.php
Outdated
| } | ||
|
|
||
| return 0; | ||
| } catch (InfectionException $e) { |
There was a problem hiding this comment.
I think you can skip this whole try/catch: the Console command handles very well exceptions:
- the return value is non 0 when one occurs
- you get the exception type & message
- depending of the verbosity you also get the stack trace
src/Command/InfectionCommand.php
Outdated
| return new DotFormatter($this->output); | ||
| } | ||
|
|
||
| throw new \InvalidArgumentException('Incorrect formatter. Possible values: dot, progress'); |
There was a problem hiding this comment.
- what about when there is no formatter specified?
- I would put in quotes
dotandprogress:'Incorrect formatter. Possible values: "dot", "progress"' - missing use statement
There was a problem hiding this comment.
what about when there is no formatter specified?
dot will be by default. fifth parameter
->addOption(
'formatter',
null,
InputOption::VALUE_REQUIRED,
'Output formatter. Possible values: dot, progress',
'dot'
)
missing use statement
not sure what do you mean. If you suggest to add InvalidArgumentException to the use section I don't really like this.
There was a problem hiding this comment.
If you suggest to add InvalidArgumentException to the use section I don't really like this.
Why? It's a dependency like another. The only difference between \InvalidArgumentException and \Acme\InvalidArgumentException would be that the later is namespaced. But for both I think it's more readable to import them with a use statement like everything
Btw if you wanna roll some optim, using fully-qualified calls is always better and the only alternative to it is use statements (func, class & const). I plan to have that with php-scoper in the PHAR but if you want to have it outside of the PHAR send I think waiting/helping for the PHP-CS-Fixer to get that would be good
src/Command/InfectionCommand.php
Outdated
| $eventDispatcher->addSubscriber(new Listener\MutationGeneratingConsoleLoggerSubscriber($this->output, $mutationGeneratingProgressBar)); | ||
| $eventDispatcher->addSubscriber(new Listener\MutantCreatingConsoleLoggerSubscriber($this->output, $mutantCreatingProgressBar)); | ||
| $eventDispatcher->addSubscriber(new Listener\MutationTestingConsoleLoggerSubscriber($this->output, $this->getOutputFormatter(), $metricsCalculator, $this->getContainer()->get('diff.colorizer'), $this->input->getOption('show-mutations'))); | ||
| $eventDispatcher->addSubscriber(new Listener\FileLoggerSubscriber\BaseFileLoggerSubscriber($this->getContainer()->get('infection.config'), $metricsCalculator, $this->getContainer()->get('filesystem'), (int) $this->input->getOption('log-verbosity'))); |
There was a problem hiding this comment.
I must say I'm not a big fan of those super long lines that requires horizontal scrolling, what about:
$subscribers = [
new Listener\InitialTestsConsoleLoggerSubscriber(
$this->output, $initialTestsProgressBar,
$testFrameworkAdapter
),
// ...
];
foreach($subscribers as $subscriber) {
$eventDispatcher->addSubscriber($subscriber);
}
src/Command/InfectionCommand.php
Outdated
| @@ -162,19 +367,18 @@ private function buildDynamicDependencies(InputInterface $input) | |||
| * @param InputInterface $input | |||
| * @param OutputInterface $output | |||
| * | |||
| * @throws \Exception | |||
| * @throws \RuntimeException | |||
There was a problem hiding this comment.
I wouldn't document this throws, anything in PHP can throw a RuntimeException
|
|
||
| class Application extends BaseApplication | ||
| { | ||
| const NAME = 'Infection - PHP Mutation Testing Framework'; |
There was a problem hiding this comment.
instead of having a constant here what about putting it in the constructor in bin/infection? It allows to pick a different name during the tests for example if you want to
There was a problem hiding this comment.
you could also leverage default values in the constructor instead:
function __construct(
Container $container,
string $name = 'Infection - PHP Mutation Testing Framework',
string $version = '@package_version@'
) {
// ...
}There was a problem hiding this comment.
yeah I thought about it but decided to extract this strings into constants. As for me it's static information which won't ever change.
@borNfreee what do you think?
give me a shout if you think that we should put these strings into constructor by default or move them into bin/infection
There was a problem hiding this comment.
As for me it's static information which won't ever change.
I have no idea why we need to test/replace this string. It's up to you guys. Current implementation is ok IMO
There was a problem hiding this comment.
I like to do it like that because avoids a constant declaration and is aligned with a couple of other projects I check. But I really don't think it matters all to much it's just personal preferences so roll with whatever you prefer :)
| { | ||
| $this->buildDynamicDependencies($input); | ||
|
|
||
| $output->writeln(self::LOGO); |
There was a problem hiding this comment.
Composer among a few other projects were putting it in getHelp() instead. Maybe it's just personal preferences I didn't really notice any difference in practice. Eventually it may be a better practice in case you leverage parallelism? Well whatev, if you're happy with it we can keep it, otherwise we can align otherselves to the others
There was a problem hiding this comment.
composer doesn't show logo when you execute composer install for example. I'd say that we can leave it as is right now
|
Could you please rebase @sidz to resolve conflicts? |
|
@borNfreee done |
|
Thank you @sidz 👍 🎉 |
The main my idea of this PR is avoiding of creation
applicationservice as it's not needed in the container.But here is a couple improvements what this PR is bringing as well:
InfectionCommandbut with this PR Logo will be shown for any command.SelfUpdateCommandis included forpharpackage only as it doesn't make sense to have it for both cases.