Fix addon filtering when no category present #360
Conversation
Signed-off-by: Steven Sheehy <steven.sheehy@gmail.com>
|
Thanks for this! Love that you've put effort into the tests. I should be able to take a look some time this week. |
unexpectedpanda
left a comment
unexpectedpanda
left a comment
There was a problem hiding this comment.
Thanks for the pull request! I've provided some comments, but I wouldn't go making any changes just yet. There are things in here for me to consider before this can proceed further.
.gitignore
Outdated
| !.gitignore | ||
| !.dev | ||
| !tests/**/*.dat | ||
| /_internal/ |
There was a problem hiding this comment.
Are you building the Pyinstaller binary directly in the root? I'm unsure why this is added.
There was a problem hiding this comment.
It looks like I might have downloaded the installer zip from the release page and unzipped it in the source directory. Which I didn't realize happened so thought it was a byproduct of the build so added the gitignore entry. I can remove this entry.
modules/titletools.py
Outdated
|
|
||
| # Other tags | ||
| self.addons: Pattern[str] = re.compile( | ||
| '\\((Addon( for XBLA)?|DLC|(Title )?Update)\\)', flags=re.I |
There was a problem hiding this comment.
At the surface level I'm happy to consider DLCs as add-ons, but updates not so much. If filtering out updates is to be added to Retool at all, that's functionality that's better split into its own feature. You're also going to need something a lot more comprehensive for the variable syntax used for updates across different DAT files.
It seems like your ultimate intent here is to provide functionality to filter out everything except the base games?
There's a bigger question here of should DLC exclusion be its own thing and not bundled into add-on exclusion -- something that I'm going to have to spend some time thinking about and testing against the existing add-on titles to make sure unexpected things don't turn up.
There are a few more things to consider beyond this tweak. Namely that there are GUI impacts in terms of tooltips, potentially new checkboxes to add, and there's the documentation, too.
For the regex, I tend to use non-capturing groups where possible:
\\((?:Addon(?: for XBLA)?|DLC)\\)
There was a problem hiding this comment.
Correct, I'm attempting to filter out everything except the base game. Probably we need some separate flag for updates like you said. I've adjusted the regex as recommended.
| <header> | ||
| <name>Retool - Exclusions (Retool)</name> | ||
| <description>Retool - Exclusions (1) (-x) [-aAbBcdDefkmMopPruv] (2024-02-24 00-00-00) (Retool)</description> | ||
| <description>Retool - Exclusions (1) [-aAbBcdDefkmMopPruv] (2024-02-24 00-00-00) (Retool)</description> |
There was a problem hiding this comment.
Legacy mode (indicated by the -x, and enabled with --legacy) is important for testing. It helps to make sure that clone assignments are working correctly, and you didn't get an end result by luck. It also adds the release tags you've removed here.
In this particular instance there's only one file with no clone relationship established, but it's good to still set this baseline in case the test changes in the future and there's more to keep track of.
It's not easy to make the link from -x to --legacy, I know. It's a relic of how Retool use to enable legacy mode (with -x), yet putting legacy in a filename is way too long. It's something that I'll eventually look into.
There was a problem hiding this comment.
Thanks, I've added it back. However, the tests don't pass with it present which is confusing for a newcomer. Perhaps there's some way you can modify the tests to pass it automatically or update the docs so it passes with hatch run integration:exclusions?
| <rom name="Test Title 52 (USA) (DLC).cue" size="1000" crc="00000000" md5="00000000000000000000000000000000" sha1="0000000000000000000000000000000000000000"/> | ||
| <rom name="Test Title 52 (USA) (DLC).bin" size="1000" crc="00000000" md5="00000000000000000000000000000000" sha1="0000000000000000000000000000000000000000"/> | ||
| </game> | ||
| <game name="Test Title 53 (USA) (update)"> |
There was a problem hiding this comment.
Be careful with capitalization. Even if it's mock data, we're trying to get close to the real thing. update > Update, addon for xbla > Addon for XBLA.
There was a problem hiding this comment.
It was intentional. The regexes are case insensitive so I made some lowercase to test that code flow. I can make it exact if that's how you prefer.
| <rom name="Test Title 0 (USA).cue" size="1000" crc="00000000" md5="00000000000000000000000000000000" sha1="0000000000000000000000000000000000000000"/> | ||
| <rom name="Test Title 0 (USA).bin" size="1000" crc="00000000" md5="00000000000000000000000000000000" sha1="0000000000000000000000000000000000000000"/> | ||
| </game> | ||
| <!-- This title should be removed when add-ons are excluded --> |
There was a problem hiding this comment.
There are now multiple add-ons, so this comment should be updated to take the plural into account.
Okay, looking through the existing titles categorized as Let's keep updates separate for now, as I still think there's more to solve there. After you've made the changes here, I'll have a follow up PR for the GUI tweaks. |
Signed-off-by: Steven Sheehy <steven.sheehy@gmail.com>
|
Thanks for the review. It's ready for another round when you get a chance. |
2 participants
This PR fixes add-on category filter when there's no
<category>addon</category>in the DAT like in No-Intro. The add-ons should be filtered using keywords in the title that are wrapped in parenthesis like(DLC)or(Addon).The exclusions test didn't work on main but I made a stab at making them work and adding tests.
Fixes #359