Skip to content
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 Progpilot task #507

Merged
merged 3 commits into from
Jun 29, 2018
Merged

Add Progpilot task #507

merged 3 commits into from
Jun 29, 2018

Conversation

eric-therond
Copy link
Contributor

Q A
Branch master
Bug fix? no
New feature? yes
BC breaks? no
Deprecations? no
Documented? yes
Fixed tickets

Add Progpilot task

New Task Checklist:

  • Is the README.md file updated?
  • Are the dependencies added to the composer.json suggestions?
  • Is the doc/tasks.md file updated?
  • Are the task parameters documented?
  • Is the task registered in the tasks.yml file?
  • Does the task contains phpspec tests?
  • Is the configuration having logical allowed types?
  • Does the task run in the correct context?
  • Is the run() method readable?
  • Is the run() method using the configuration correctly?
  • Are all CI services returning green?

Copy link
Contributor

@Landerstraeten Landerstraeten left a comment

Choose a reason for hiding this comment

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

Looks good to me

***Composer***

```
composer config minimum-stability dev
Copy link
Contributor

Choose a reason for hiding this comment

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

Shouldn't we propose a released version instead?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Unfortunately, progpilot relies on ircmaxell/php-cfg which doesn't currently have a stable version

$resolver = new OptionsResolver();
$resolver->setDefaults(
[
'config_file' => '.progpilot/configuration.yml',
Copy link
Contributor

Choose a reason for hiding this comment

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

Isn't it better to set this value to null so that the defaults from the cli tool are being used?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Defaults values from the tool are also used when the configuration file specified in the cli doesn't exist so if someone doesn't have a file .progpilot/configuration.yml the defaults values from the tool will be used and if someone has a file .progpilot/configuration.yml but has forgotten to define it in the parameters of the task the values of .progpilot/configuration.yml will be used.

$arguments = $this->processBuilder->createArgumentsForCommand('progpilot');

$arguments->addOptionalArgumentWithSeparatedValue('--configuration', $config['config_file']);
$arguments->addFiles($files);
Copy link
Contributor

Choose a reason for hiding this comment

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

Maybe better not to specify files during run context?

Copy link
Contributor Author

Choose a reason for hiding this comment

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

If I comment the line 67 the commited files are not analyzed by the tool

@veewee veewee modified the milestones: Version 0.14.1, Version 0.14.2 May 25, 2018
@veewee veewee merged commit 78f785a into phpro:master Jun 29, 2018
@veewee
Copy link
Contributor

veewee commented Jun 29, 2018

Sounds reasonable. Thanks for the feedback!

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

3 participants