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

Ticket #1157: support specific filenames as trigger for Shell. #1167

Draft
wants to merge 1 commit into
base: v2.x
Choose a base branch
from
Draft
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
12 changes: 11 additions & 1 deletion doc/tasks/shell.md
Original file line number Diff line number Diff line change
Expand Up @@ -10,6 +10,7 @@ grumphp:
shell:
scripts: []
triggered_by: [php]
triggered_by_filenames: []
```

**scripts**
Expand Down Expand Up @@ -41,5 +42,14 @@ grumphp:
*Default: [php]*

This option will specify which file extensions will trigger the shell tasks.
By default, Shell will be triggered by altering a PHP file.
By default, Shell will be triggered by altering a PHP file.
You can overwrite this option to whatever file you want to use!


**triggered_by_filenames**

*Default: []*

This option will specify which files will trigger the shell tasks.
By default, Shell will not be triggered by specific files.
You can overwrite this option to whatever file you want to use!
2 changes: 2 additions & 0 deletions src/Task/AbstractParserTask.php
Original file line number Diff line number Diff line change
Expand Up @@ -50,10 +50,12 @@ protected static function sharedOptionsResolver(): OptionsResolver
$resolver = new OptionsResolver();
$resolver->setDefaults([
'triggered_by' => [],
'triggered_by_filenames' => [],
Copy link
Contributor

Choose a reason for hiding this comment

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

not sure why this one is altered? It's not being used anywhere?

'ignore_patterns' => [],
]);

$resolver->addAllowedTypes('triggered_by', ['array']);
$resolver->addAllowedTypes('triggered_by_filenames', ['array']);
$resolver->addAllowedTypes('ignore_patterns', ['array']);

return $resolver;
Expand Down
9 changes: 8 additions & 1 deletion src/Task/Shell.php
Original file line number Diff line number Diff line change
Expand Up @@ -4,6 +4,7 @@

namespace GrumPHP\Task;

use GrumPHP\Collection\FilesCollection;
use GrumPHP\Exception\RuntimeException;
use GrumPHP\Formatter\ProcessFormatterInterface;
use GrumPHP\Runner\TaskResult;
Expand All @@ -26,6 +27,7 @@ public static function getConfigurableOptions(): ConfigOptionsResolver
$resolver->setDefaults([
'scripts' => [],
'triggered_by' => ['php'],
'triggered_by_filenames' => [],
]);

$resolver->addAllowedTypes('scripts', ['array']);
Expand Down Expand Up @@ -59,7 +61,12 @@ public function canRunInContext(ContextInterface $context): bool
public function run(ContextInterface $context): TaskResultInterface
{
$config = $this->getConfig()->getOptions();
$files = $context->getFiles()->extensions($config['triggered_by']);
$files = new FilesCollection(
Copy link
Contributor

Choose a reason for hiding this comment

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

$files isn't really being used anywhere.
Instead of building the list of files, it might make more sense to count() the amount of files in either trigger.

That way we don't have to do all these lookups all of the time.

array_merge(
$context->getFiles()->extensions($config['triggered_by'])->toArray(),
$context->getFiles()->names($config['triggered_by_filenames'])->toArray(),
)
);
if (0 === \count($files)) {
return TaskResult::createSkipped($this, $context);
}
Expand Down
3 changes: 3 additions & 0 deletions test/Unit/Task/ShellTest.php
Original file line number Diff line number Diff line change
Expand Up @@ -27,6 +27,7 @@ public function provideConfigurableOptions(): iterable
[
'scripts' => [],
'triggered_by' => ['php'],
'triggered_by_filenames' => [],
Copy link
Contributor

Choose a reason for hiding this comment

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

We'll also need a test-case that covers how the task behaves on the newly added configuration.

]
];
yield 'string_script' => [
Expand All @@ -38,6 +39,7 @@ public function provideConfigurableOptions(): iterable
['phpunit']
],
'triggered_by' => ['php'],
'triggered_by_filenames' => [],
]
];
yield 'array_script' => [
Expand All @@ -51,6 +53,7 @@ public function provideConfigurableOptions(): iterable
['phpunit', 'tests']
],
'triggered_by' => ['php'],
'triggered_by_filenames' => [],
]
];
}
Expand Down