HTML Search: Fix multiple term matching edge case

[wlach]

Subject: Fix multiple term matching edge case

Feature or Bugfix

• Bugfix

Purpose

Fixes multiple matches getting accidentally overwritten in certain rare cases

Detail

In certain rare cases, term matches could get overwritten due to a logic bug in the search code. Fix this and add a test.

Relates

Closes #11959

[jayaddison]

Two nitpicks with the test case:

• Could you omit the objtypes and any other non-essential properties of the test fixture?
• Adding a newline between the setup phase of the test (creating and setting the index) and the assertion/expectation phase (creating the expected hits list and running expect) could provide a useful hint to readers about how the test is structured.

[wlach]

Two nitpicks with the test case:

* Could you omit the `objtypes` and any other non-essential properties of the test fixture?

* Adding a newline between the setup phase of the test (creating and setting the index) and the assertion/expectation phase (creating the expected `hits` list and running `expect`) could provide a useful hint to readers about how the test is structured.

Done! For future reference if you have suggestions using GitHub's "suggest" feature in a comment on the diff is a little easier for me to apply -- see step 6 here:

https://docs.github.com/en/pull-requests/collaborating-with-pull-requests/reviewing-changes-in-pull-requests/commenting-on-a-pull-request#adding-comments-to-a-pull-request

[jayaddison]

This looks good to me 👍

[jayaddison]

I've been fairly cautious about reviewing these search changes because I'd like to understand them in some detail. This one still looks good to me.

One minor suggestion that I have -- frustratingly, GitHub doesn't allow suggestions on line ranges that include unmodified lines, it seems -- would be to aim for consistency with the creation of scoreMap when building fileMap here. That would be something like:

        if (!fileMap.has(file)) fileMap.set(file, [word]);
        else if (fileMap.get(file).indexOf(word) === -1) fileMap.get(file).push(word);

...however: that's optional, and again I think this pull request is in good shape.

[wlach]

One minor suggestion that I have -- frustratingly, GitHub doesn't allow suggestions on line ranges that include unmodified lines, it seems -- would be to aim for consistency with the creation of scoreMap when building fileMap here. That would be something like:
...

In JavaScript it's generally considered best practice to always use curly braces in a conditional clause: https://eslint.org/docs/latest/rules/curly -- especially when there's an else involved.

That said, this would be consistent with the style elsewhere in this file. I guess I'll change it, this PR needs a changelog entry anyway.

[jayaddison]

Thanks again @wlach 👍

[wlach]

@jayaddison @picnixz Ok I rebased this on top of master / #11958 so I think it's ready to go in too.

[picnixz]

I think it's fine. It'll be the last PR that I'll review for the next 10 days (I'm travelling).

Thank you. If there is some critical things to do next week, ping me or chrisjsewell or jfbu which have write permissions.

[wlach]

I think it's fine. It'll be the last PR that I'll review for the next 10 days (I'm travelling).

Thank you. If there is some critical things to do next week, ping me or chrisjsewell or jfbu which have write permissions.

No problem, I'll probably just wait for you/others to take a look at the remaining work on their own schedule. If it's been a while I will ping.