Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

fix(SharedHubStrategy): exclude Meta and Misc hubs #3131

Merged
merged 7 commits into from
Jan 29, 2025
Merged

fix(SharedHubStrategy): exclude Meta and Misc hubs #3131

Changes from 1 commit
Commits
Show all changes
7 commits
Select commit Hold shift + click to select a range
772d5a0
fix(SharedHubStrategy): exclude Meta and Misc hubs
wescopeland Jan 25, 2025
761caa8
fix: address feedback
wescopeland Jan 26, 2025
db40edf
fix: address feedback
wescopeland Jan 26, 2025
7acb411
chore: revert some stuff
wescopeland Jan 26, 2025
4b60090
Merge branch 'master' into shared-hub-strategy-exclude-some-kinds
wescopeland Jan 29, 2025
d49d9c0
fix: address pr feedback
wescopeland Jan 29, 2025
54b02ad
Merge branch 'master' into shared-hub-strategy-exclude-some-kinds
wescopeland Jan 29, 2025
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 2 additions & 0 deletions app/Platform/Services/GameSuggestions/Strategies/SharedHubStrategy.php
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -36,6 +36,8 @@ public function select(): ?Game
->whereHas('games', function ($query) {
$query->where('game_id', '!=', $this->sourceGame->id); // needs other games too!
})
->where('Title', 'NOT LIKE', "%Meta%")
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be NOT LIKE "[Meta%" or you'll exclude hubs like "[Series - Metal Gear]" and "[Series - Twisted Metal]".

Copy link
Member Author

@wescopeland wescopeland Jan 26, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I don't know that we can guarantee hubs will always be encased with square brackets. The problem you're describing definitely makes sense, but I think "%Meta - %" may be a more future-proofed target string.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The %Misc. -% filter will work, but %Meta -% will miss anything [Meta|QA -, [Meta|Art - , or [Meta|DevComp -

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Good catch, I forgot about those.

I have reservations about adding more "NOT LIKE" filters on the query. I've combined everything into a single:

->whereRaw("game_sets.title NOT REGEXP '^(Meta[| -]|Misc\\.[ -])'")

which should be a bit more performant. Note that the square brackets in the pattern are not checking for the presence of square brackets in the title. They're defining character classes for conditionals in the pattern.

SQLite doesn't support REGEXP, so I needed to add some conditional logic for tests.

Copy link
Member

@Jamiras Jamiras Jan 28, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Unless I made a typo, this is still matching the aforementioned series names.

select title from game_sets where title like '%meta%'
and title not regexp '^(Meta[| -]|Misc\\.[ -])';

I think it's the leading ^. Doesn't that imply that the Meta must start the string? And we know the strings all start with an open bracket.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No typo on your end, I messed up. I think I have things in a better place now in latest.

->where('Title', 'NOT LIKE', "%Misc%")
Jamiras marked this conversation as resolved.
Show resolved Hide resolved
->inRandomOrder()
->first();

Expand Down