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

Provide option to run a single/list of tasks from the config #587

Merged
merged 8 commits into from
May 16, 2019

Conversation

paslandau
Copy link

@paslandau paslandau commented Jan 7, 2019

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

Fixes #580

* @param string $value
* @return string[]
*/
protected function parseCommaSeparatedOption(string $value)
Copy link
Contributor

@veewee veewee Jan 7, 2019

Choose a reason for hiding this comment

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

It is better to move this method to the \GrumPHP\Util\Str class.
Maybe something like:

public static function explodeWithCleanup(string $delimiter, string $string): array

This way the logic can be tested in a normal way and it is reusable if we want to add more comma separated options?

@Landerstraeten Landerstraeten changed the base branch from grumpy-seventies to master February 28, 2019 15:21
@Landerstraeten
Copy link
Contributor

Hi @paslandau. Thanks for your PR! Can you rebase against master? I will review it soon.

@Landerstraeten Landerstraeten force-pushed the 580_seventies branch 2 times, most recently from 55dadde to 51bab84 Compare April 19, 2019 11:53
Copy link
Contributor

@veewee veewee left a comment

Choose a reason for hiding this comment

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

Code looks good. I've added 2 small remarks.

@@ -0,0 +1,63 @@
<?php
Copy link
Contributor

Choose a reason for hiding this comment

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

Add e2e tests instead; (this namespace is not in a testsuite)

public function filterByTaskNames(array $tasks): self
{
if (empty($tasks)) {
return new self($this->toArray());
Copy link
Contributor

Choose a reason for hiding this comment

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

return $this; ? No need to create a new instance in that case right?

@veewee veewee changed the title Fixes #580 against seventies branch Provide option to run a single/list of tasks from the config May 16, 2019
@Landerstraeten Landerstraeten merged commit 2d6d34c into phpro:master May 16, 2019
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.

Provide option to run a single/list of tasks from the config
3 participants