chore: refactor UrlMatcher #1720
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
mxschmitt
commented
Dec 19, 2024
playwright/src/main/java/com/microsoft/playwright/impl/UrlMatcher.java
Outdated
Show resolved
Hide resolved
yury-s
reviewed
Dec 19, 2024
playwright/src/main/java/com/microsoft/playwright/impl/UrlMatcher.java
Outdated
Show resolved
Hide resolved
playwright/src/main/java/com/microsoft/playwright/impl/UrlMatcher.java
Outdated
Show resolved
Hide resolved
| static UrlMatcher any() { | ||
| return new UrlMatcher((Object) null, null); | ||
| } | ||
| private final URL baseURL; |
There was a problem hiding this comment.
Why not resolve the pattern in the constructor and avoid plumbing base url all the way through?
There was a problem hiding this comment.
We need this later on when I'll add this check:
playwright/src/main/java/com/microsoft/playwright/impl/UrlMatcher.java
Outdated
Show resolved
Hide resolved
mxschmitt
force-pushed
the
refactore-UrlMatcher
branch
from
d564bc4 to
fb252dc
Compare
yury-s
reviewed
Dec 19, 2024
playwright/src/main/java/com/microsoft/playwright/impl/UrlMatcher.java
Outdated
Show resolved
Hide resolved
playwright/src/main/java/com/microsoft/playwright/impl/UrlMatcher.java
Outdated
Show resolved
Hide resolved
mxschmitt
force-pushed
the
refactore-UrlMatcher
branch
from
9f6a2a8 to
e31f5d3
Compare
yury-s
reviewed
Dec 19, 2024
playwright/src/main/java/com/microsoft/playwright/impl/UrlMatcher.java
Outdated
Show resolved
Hide resolved
playwright/src/main/java/com/microsoft/playwright/impl/UrlMatcher.java
Outdated
Show resolved
Hide resolved
mxschmitt
force-pushed
the
refactore-UrlMatcher
branch
from
e31f5d3 to
97def24
Compare
yury-s
reviewed
Dec 19, 2024
| } | ||
| return Objects.equals(rawSource, that.rawSource); | ||
| return true; |
There was a problem hiding this comment.
I believe it should be return that.pattern == null && that.predicate == null && that.glob == null;, otherwise it will be equal to any other pattern.
There was a problem hiding this comment.
I think I already have it like that.
There was a problem hiding this comment.
I disagree, with the current code e.g.new UrlMatcher(null, null, null, null).equals(new UrlMatcher((String s) -> true) == true which is incorrect.
playwright/src/main/java/com/microsoft/playwright/impl/UrlMatcher.java
Outdated
Show resolved
Hide resolved
yury-s
approved these changes
Dec 27, 2024
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
Extracted from #1717. There should be no behaviour changes.
This moves
rawSourcetowards having it lazily determined which matching source it is - this helps us to align with upstream.