-
-
Notifications
You must be signed in to change notification settings - Fork 47
Fix pattern match #404
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
Fix pattern match #404
Conversation
a7e3e5e to
7d7e9c1
Compare
|
Thanks for you contributions! As you can see from the broken tests this function is used in a lot of places, and reverting to an old behavior is not a valid solution... |
|
Reverting the changes and leaving the test as is, we can see that this assertion fails: $pattern = new PatternString('SoThisIsAnExample');
$this->assertFalse($pattern->matches('*This'));However, it shouldn't fail because it shouldn't match since there is no wildcard at the end of the string. |
|
@fain182 |
|
@fain182 I believe I fixed the issues, can you check the last commits, please? |
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## main #404 +/- ##
============================================
- Coverage 94.79% 94.76% -0.04%
+ Complexity 606 604 -2
============================================
Files 69 69
Lines 1614 1604 -10
============================================
- Hits 1530 1520 -10
Misses 84 84 ☔ View full report in Codecov by Sentry. 🚀 New features to boost your workflow:
|
|
@hgraca i need this ;-; Can we merge it? |
|
@d-cichon as far as I'm concerned, yes. However, I'm not a maintainer, I can't merge anything. |
7f6ae0d to
f628a1b
Compare
f628a1b to
0e965be
Compare
AlessandroMinoccheri
left a comment
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM 👍
|
@fain182 @micheleorselli it seems OK for me, any other opinions? |
| $resideInNamespace = false; | ||
| foreach ($this->namespaces as $namespace) { | ||
| if ($theClass->namespaceMatches($namespace)) { | ||
| if ($theClass->namespaceMatches($namespace.'*')) { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
just out of curiosity, why do we need to add this? cc @AlessandroMinoccheri
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Its from a long time ago, but I guess it was because if a class is in a subnamespace is still in the given namespace.
However, looking at it now, it's out of scope fo this PR, and not having the * is a better option because we can match the namespace exactly or provide the namespace with a * which will match subnamespaces as well.
So, Im adding a fixup commit to remove this change.
The test ResideInOneOfTheseNamespacesTest::test_it_should_match_namespace_and_descendants will fail if this is not there, as it seems this expression has been designed to match the namespace or its descendants.
0e965be to
1d91538
Compare
|
I rebased and force pushed |
At some point this was correct and then changed into a bug.
I am reverting the change that introduced the bug.