Fix search commands reporting only the first matching player#414
Conversation
- Resolves Jikoo#413 - Regression introduced by: Jikoo#311 Before the regression, `SearchInvCommand` used two nested loops: an outer loop over online players and an inner loop over the inventory contents. The `break` lived in the **inner** loop, ending the per-item count-up once the requested threshold was reached. That commit extracted the inner loop into `SearchHelper.findMatch(...)` but left the `break` behind. It now sits directly inside the **outer** `for (Player player : ...)` loop, so the entire player scan terminates at the first match. I could be wrong about the intentionality of this `break` statement; however, in my eyes, this looks correct!
|
Off-topic: In other news @Jikoo (not sure if this is intentional), I think it might be good to have the two upstream (behind) commits rebased and dropped into your OpenInv fork. They appear a little misleading at times! (Users can see that this is a fork via commits ahead, exclusively) |
|
Oof, thanks for the catch on that logical error. Do people really worry that much about two commits not being present? The commits to upstream are purely for the location change - they introduced a section in the readme and removed all other Actions in favor of ones that close issues/PRs and direct here. I don't really want to pull them in because then I'd have to either revert them or effectively revert them during the merge. They also make it easier for me to make PRs against this repo because they cause merge conflicts. GitHub makes the "open PR" button on forks open against the upstream by default rather than the location you click in. That's great for people forking this repo to contribute, but means mine always open against the original even when I try to initiate them here. The only way to stop that is to submit a request to GitHub to sever this repo's relationship with the original, which probably would hurt discoverability more. I don't think GH by default has much discoverability for forks unless you look at the network graph, which this repo usually is in the top couple entries of depending on how recently people who forked from it updated. For powerusers, Useful Forks (which does also have a Chrome extension... not that I use Chrome any more since they neutered uBlock) does report this repo as being the top of the list. /e: Does make me wonder if the actions still work, honestly. I think GH has upgraded to the point where they might actually be broken and need an update. Fortunately no one has opened an issue there in a couple years. I guess I could go back and drop those commits if they no longer work, maybe edit the last release to point here or something instead. |
|
Thank you! And eh, it’s just for readability sake for complete indication that upstream was superseded. You’d more or less just rebase and only accept your changes and it’d label the very old comments at the earliest point in your fork |
2 participants
Before the regression,
SearchInvCommandused two nested loops: an outer loop over online players and an inner loop over the inventory contents. Thebreaklived in the inner loop, ending the per-item count-up once the requested threshold was reached.That commit extracted the inner loop into
SearchHelper.findMatch(...)but left thebreakbehind. It now sits directly inside the outerfor (Player player : ...)loop, so the entire player scan terminates at the first match.I could be wrong about the intentionality of this
breakstatement; however, in my eyes, this looks correct!