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

feat(upgrade): use multiple routes #70

Merged
merged 3 commits into from
Aug 30, 2022
Merged

Conversation

raf262
Copy link
Contributor

@raf262 raf262 commented Aug 3, 2022

Why

I want to add multiple route for the same configuration and also exclude a route

How

Edit the configuration and code to use multiple route, no breaking changes

README.md Outdated Show resolved Hide resolved
@raf262 raf262 force-pushed the feat/filter-multiple-route branch 4 times, most recently from 15bd958 to 959ff29 Compare August 4, 2022 09:23
@raf262 raf262 marked this pull request as ready for review August 4, 2022 09:32
@raf262 raf262 requested a review from a team as a code owner August 4, 2022 09:32
@raf262 raf262 force-pushed the feat/filter-multiple-route branch from 959ff29 to fb8fc26 Compare August 4, 2022 12:59
Copy link
Member

@Oliboy50 Oliboy50 left a comment

Choose a reason for hiding this comment

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

OK for a new major release

@raf262 raf262 force-pushed the feat/filter-multiple-route branch 2 times, most recently from f06936c to 5969742 Compare August 4, 2022 13:58
@raf262 raf262 requested a review from omansour August 16, 2022 14:27
@Fabex
Copy link
Contributor

Fabex commented Aug 17, 2022

I think that we can avoid new major release by allowing the 2 options (route and routes).

@raf262 raf262 force-pushed the feat/filter-multiple-route branch 2 times, most recently from dda0a7d to b7d9f0f Compare August 23, 2022 11:55
README.md Outdated Show resolved Hide resolved
Copy link

@L-SeeoX L-SeeoX left a comment

Choose a reason for hiding this comment

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

LGTM & tested

@raf262 raf262 requested a review from Fabex August 29, 2022 14:34
Comment on lines +77 to +78
if (array_key_exists('route', $config) && array_key_exists('routes', $config)) {
throw new ParseException(sprintf('You can\'t use both "route" and "routes" parameter from filter "%s"', $name));
Copy link
Contributor

Choose a reason for hiding this comment

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

I was not expected this. instead of create a 2 if condition for route(s) config you can use the xor operator.
so in the first condition you can replace (!array_key_exists('route', $config) && !array_key_exists('routes', $config)) by (!(array_key_exists('route', $config) xor array_key_exists('routes', $config)))

Copy link
Contributor Author

Choose a reason for hiding this comment

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

We saw this together like a family, it's ok to have both if

@Fabex Fabex merged commit e187c68 into master Aug 30, 2022
@Fabex Fabex deleted the feat/filter-multiple-route branch August 30, 2022 08:44
@raf262 raf262 requested a review from emyrtille August 30, 2022 09:28
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants