Fix regex skipping over all src except the last #3021
Conversation
Adapting HTML_MEDIA_TAGS to allow for `>` inside '' and "" led to multiple images inside a field sometimes being disregarded and marked as "unused". This seems to have been caused by a missing lazy (?) quantifier for the regex part skipping over non-`>`.
|
Not sure why the build failed. Seems to be unrelated? Tests pass locally. |
| let seen = normalize_and_maybe_rename_files_helper(&mut checker, field); | ||
| assert!(seen.contains("foo-sd.jpg")); | ||
| assert!(seen.contains("bar-sd.jpg")); | ||
|
|
There was a problem hiding this comment.
Would it make sense to add some tests for where there's whitespace, a newline or text (including other html tags) between the image tags? Also maybe tests for when there are more than two images?
For the particular bug that this PR fixes all that makes no difference. But in my mind, the tests should cover more than the bug that we just fixed, but also unforeseen future bugs. On the other hand that would add a lot of boilerplate code. I'm haven't done much Anki development, so not sure what the testing guidelines here are...
There was a problem hiding this comment.
For the issue at hand, the tests added are the minimum retention test addressing this particular fix. There are three because the bug only showed up on subsequent usage of single or double quotes. That's all a retention test should do as far as I'm concerned.
I agree it would be great to have more tests in general. How detailed the tests should be is a matter of debate I suppose. Surely there are lower hanging fruits.
There was a problem hiding this comment.
Thanks for explaining, that's fine with me.
Compiling finished, I can't reproduce the bug on this PR, thanks 👍 |
See #2918
Adapting HTML_MEDIA_TAGS to allow for
>inside '' and "" led to multiple images inside a field sometimes being disregarded and marked as "unused". This seems to have been caused by a missing lazy (?) quantifier for the regex part skipping over non->.@meliache Thanks for finding this! If you can, please check if this resolves the bug from your end.