Skip to content

PHP Config - Add Profile filters #1556

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.

Sign up for GitHub

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

Jump to bottom
Merged
acoulton merged 3 commits into from
Dec 4, 2024
Merged

PHP Config - Add Profile filters #1556

acoulton merged 3 commits into from
Dec 4, 2024

Conversation

@loic425
Copy link
Contributor

@loic425 loic425 commented Dec 3, 2024 •
edited
Loading

Referenced issue #1539

use Behat\Config\Config;
use Behat\Config\Profile;
use Behat\Config\Filter\NameFilter;
use Behat\Config\Filter\TagFilter;

return (new Config())
    ->withProfile(
        (new Profile('default'))
            ->withFilter(new NameFilter('name1'))
            ->withFilter(new TagFilter('tag2'))
  )
;

@loic425 loic425 force-pushed the php-config-gherkin-filters branch from 3f7b9c3 to 8d174d6 Compare December 4, 2024 07:50
@loic425
Copy link
Contributor Author

loic425 commented Dec 4, 2024

@acoulton @carlos-granados
I think this is now ready to review. I prefer my today implementation.

loic425
loic425 commented Dec 4, 2024
View reviewed changes
src/Behat/Config/Gherkin/Filter/FilterInterface.php Outdated
{
public function name(): string;

public function value(): mixed;
Copy link
Contributor Author

@loic425 loic425 Dec 4, 2024 •
edited
Loading

Choose a reason for hiding this comment

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

the interface value returns a mixed but the other current filters return a string.
I'm not aware if some other filters can have other value types. So, I've specified mixed on the interface but tell me if I am wrong.

Copy link
Contributor

@carlos-granados carlos-granados Dec 4, 2024

Choose a reason for hiding this comment

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

Right now the types of filters is limited and there is no way to add new filters, so I would say this can be a string

Copy link
Contributor Author

@loic425 loic425 Dec 4, 2024

Choose a reason for hiding this comment

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

done

carlos-granados
carlos-granados reviewed Dec 4, 2024
View reviewed changes
Copy link
Contributor

@carlos-granados carlos-granados left a comment

Choose a reason for hiding this comment

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

Some comments

src/Behat/Config/Gherkin/Filter/FilterInterface.php Outdated

declare(strict_types=1);

namespace Behat\Config\Gherkin\Filter;
Copy link
Contributor

@carlos-granados carlos-granados Dec 4, 2024

Choose a reason for hiding this comment

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

The filters can also be applied on the suite level, so perhaps it would be better to remove the Gherkin part of the namespace? I know that the files that implement the filtering are in this Gherkin namespace but I don't think that forces us to use the same here

Copy link
Contributor Author

@loic425 loic425 Dec 4, 2024

Choose a reason for hiding this comment

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

done

src/Behat/Config/Gherkin/Filter/FilterInterface.php Outdated
{
public function name(): string;

public function value(): mixed;
Copy link
Contributor

@carlos-granados carlos-granados Dec 4, 2024

Choose a reason for hiding this comment

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

Right now the types of filters is limited and there is no way to add new filters, so I would say this can be a string

src/Behat/Config/Gherkin/SimpleFilter.php
@@ -0,0 +1,26 @@
<?php
Copy link
Contributor

@carlos-granados carlos-granados Dec 4, 2024

Choose a reason for hiding this comment

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

I don't think this is needed. If you want to create one of the existing filters just use the corresponding class. And any new filters that you add will not be handled, Behat will throw an exception

Copy link
Contributor

@carlos-granados carlos-granados Dec 4, 2024

Choose a reason for hiding this comment

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

Unless perhaps all previous filters extend this SimpleFilter with most of the functionality delegated to it

Copy link
Contributor Author

@loic425 loic425 Dec 4, 2024

Choose a reason for hiding this comment

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

Yes, I forgot to remove it. I think we can start simple without it at all.

Copy link
Contributor Author

@loic425 loic425 Dec 4, 2024

Choose a reason for hiding this comment

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

I've just removed it.

carlos-granados
carlos-granados approved these changes Dec 4, 2024
View reviewed changes
Copy link
Contributor

@carlos-granados carlos-granados left a comment

Choose a reason for hiding this comment

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

LGTM, thanks @loic425

acoulton
acoulton approved these changes Dec 4, 2024
View reviewed changes
Copy link
Contributor

@acoulton acoulton left a comment

Choose a reason for hiding this comment

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

Great, thanks @loic425

@acoulton acoulton merged commit f2d2379 into Behat:master Dec 4, 2024
17 checks passed
@loic425 loic425 deleted the php-config-gherkin-filters branch December 4, 2024 13:09
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

@acoulton acoulton acoulton approved these changes

@carlos-granados carlos-granados carlos-granados approved these changes

Assignees

No one assigned

Labels

None yet

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

3 participants

@loic425 @acoulton @carlos-granados