facile-it / paraunit

Run PHPUnit tests in parallel
https://engineering.facile.it/paraunit/
Apache License 2.0
140 stars 15 forks source link

[Version] Aligned version number #53

Closed ranpafin closed 7 years ago

ranpafin commented 7 years ago

This PR fixes a minor typo and align the version number shown in the CLI Header Aka Shark printer with the value found in the command help.

Jean85 commented 7 years ago

Woha, I forgot that I had that in the YML config, thanks! I had fixed it in the 0.6.2 release anyway, see: https://github.com/facile-it/paraunit/commit/0373ac0aa046fef46e67dc6c72d8fd54f03805bf

Anyway, I'm not sure if I would move that information in the YAML: I would prefer the constant, since I would like to have that information available everywhere in the project, even in the bin/paraunit, for example.

With your modification, that info is available only inside a class that is instantiated by the container, which may factor in later into the execution, especially when I will merge the coverage feature, where I delayed it, see: https://github.com/facile-it/paraunit/blob/feature-coverage-new-approach-rebased/src/Paraunit/Command/ParallelCommand.php#L58

What do you think?

ranpafin commented 7 years ago

@Jean85 it could make sense to keep it inside the class constant it's more flexible! In a sense this PR was more about keeping the values aligned.... How about we change both the console version and the shark printer to a constant call (the Service container has a way to accept class constant)?

something like this:

<parameter key="my_class.constant.value" type="constant">My_Class::CONSTANT_NAME</parameter>
`
Jean85 commented 7 years ago

Yes this seems more feasible!

Why the XML? To load the constant? Is that backward compatible up to SF 2.3?

Can't we use the constant directly in the printer, without injecting it? It would never change, so injection seems a bit over the top for this case, IMHO!

ranpafin commented 7 years ago

@Jean85 yeah the .xml file seems an overkill but it's needed to inject the class contant inside the console application.

If you look here services.yml

the system is using a container reference for the console/Application constructor:

public function __construct($name = 'UNKNOWN', $version = 'UNKNOWN')

and looking at the symfony docs the yaml is not supported for injecting class constant :(

Jean85 commented 7 years ago

@ranpafin yes it's supported, but only form 3.2 :( http://symfony.com/blog/new-in-symfony-3-2-php-constants-in-yaml-files

I would like to ascertain if the XML support for the same usage is present since 2.3, which is the oldest SF version that we currently support.

Once I've checked that, I would merge this, and I will probably go over that once more when merging the coverage feature.

Jean85 commented 7 years ago

I had to dig a bit, but' I've found that's supported! https://github.com/symfony/symfony-docs/blob/2.3/components/dependency_injection/parameters.rst#constants-as-parameters

Merging, thanks!