Minor fixes to query text pattern selector #27604
Conversation
The issue is that `Pattern` does not implement `hashCode` so we can't use it directly for hashing. It also doesn't implement `equals`, but it's already properly handled in `SelectorSpec`'s `equals` method - we should do the analogous thing in `hashCode` as well. As a bonus, do that in `toString`, too.
This is what we do for all the other `Pattern` selection criteria.
| sourceRegex.map(Pattern::flags), | ||
| queryText, | ||
| queryText.map(Pattern::pattern), | ||
| queryText.map(Pattern::flags), |
There was a problem hiding this comment.
Or maybe SelectorSpec should not implement equals/hashcode at all?
what's the use of it anyway?
(potential follow-up)
There was a problem hiding this comment.
Or maybe SelectorSpec should not implement equals/hashcode at all?
what's the use of it anyway?
Sadly, I don't know the answer to that question
There was a problem hiding this comment.
me neither, might be best to keep it this way
There was a problem hiding this comment.
if the equals is meaningless, there is no point in keeping things like this.
|
Thanks for your fixes, I noticed them today when using the classes. Can you rename queryText to queryTextRegex in the selectorspec? That seems consistent with the other patterns. (Note the textual rename on line 58) |
Yeah, I noticed that. I love consistency too, but I think it's low priority this time :) |
4 participants
Description
The issue is that
Patterndoes not implementhashCodeso we can't use it directly for hashing. It also doesn't implementequals, but it's already properly handled inSelectorSpec'sequalsmethod - we should do the analogous thing inhashCodeas well. As a bonus, do that intoString, too.Also, call
addNamedGroupsforqueryTextcriterion inStaticSelector.Additional context and related issues
Follow-up to #27129.
Release notes
(x) This is not user-visible or is docs only, and no release notes are required.
( ) Release notes are required. Please propose a release note for me.
( ) Release notes are required, with the following suggested text: