Review/gh12063 #47
Review/gh12063 #47
Conversation
Swiftb0y
force-pushed
the
review/gh12063
branch
3 times, most recently
from
269e73b to
8af74a1
Compare
Swiftb0y
force-pushed
the
review/gh12063
branch
from
8af74a1 to
261c418
Compare
| } | ||
| } | ||
| Quoted quoted; | ||
| std::tie(argument, quoted) = consumeQuotedArgument(argument, tokens); |
There was a problem hiding this comment.
nope. just had to look up what std::tie actually does.
| argument = argument.mid(1); | ||
| shouldMatchExactly = true; | ||
| mode = StringMatch::Equals; |
There was a problem hiding this comment.
Initially I wanted to make only super explicit ="string" return exact/equal matches, this change extends it to =string.
Personally I'm fine with that, but it may be a bit confusing that =string does and ="string does not find desired results.
wdyt?
There was a problem hiding this comment.
I think this is also the reason why the TextFilterNode tests fail now
https://github.com/ronso0/mixxx/actions/runs/6461369115/job/17540892333?pr=47
I'll now pick your commits, comment it and push. If that doesn't fix the tests we can revert it.
There was a problem hiding this comment.
Nope, the reason is that = is not considered below, hence :"string" is treated like :="string".
I'll push a fixup.
There was a problem hiding this comment.
Personally I'm fine with that, but it may be a bit confusing that =string does and ="string does not find desired results.
Well, I tried to achieve that = means I want to match exactly and "..." means I want to match multiple words. Those compose nicely and thus are easy to understand and apply. The only exception to this is if the string is not quoted completely. In this case I tried to emulate the behavior you implemented, even though I think we need to discuss more whether that makes sense.
I can't quite wrap my head around why
=string does and ="string does not find desired results.
Can you give me an example?
There was a problem hiding this comment.
=string does and ="string does not find desired results.
Can you give me an example?
On second thought, it's perfectly to search contains with unclosed quotes.
I reverted my fixup.
There was a problem hiding this comment.
On second thought, it's perfectly to search contains with unclosed quotes.
Right, but the implementation would be simpler if the mode didn't depend on whether the quotes were closed or not. That would get rid of the entire Quoted enum and the pair return type.
|
Thanks for this PR! Very much appreciated. |
|
Thank you, I'm sorry I left the PR in such a broken state. I got too annoyed to fix it yesterday. Thanks for doing that. |
the most important commit (72770f6) has been conflated with the a refactor a little but, but I think the result is much easier to reason about.