game pass improvements

[ohadschn]

Tested against my plan (ultimate) and region (IL) - I can confirm the exact same game count as the official Xbox web catalog (which was not the case before this change, where 100+ games were missing, including all EA Play games).

• Fixes Parametrize market for Xbox game pass catalog #35
• Fixes Import public EA Play catalog alongside Xbox Game Pass #30
• Fixes [UI] sort dropdown background is too bright #31

[sam1am]

Thanks for this PR — great work upgrading to the v3 endpoint and adding plan/market configurability. The 100+ missing games fix is a big improvement. A few changed I would like to request:

Naming convention

planInfo, collectionId, and subscriptionId use camelCase which breaks from the snake_case convention used throughout the rest of the codebase. Should be plan_info, collection_id, subscription_id.

KeyError risk on invalid plan

In get_gamepass_catalog, GAMEPASS_PLAN_MAP[plan] will raise a KeyError if plan isn't a valid key (and isn't "none"). Worth adding a .get() with a fallback or validation with an error message.

Default plan mismatch

get_resolved_xbox_gamepass_settings() defaults to "ultimate" when no setting is stored, but the settings UI defaults the dropdown to "none". This means a user who never visits settings will silently get the Ultimate catalog imported, while a user who visits settings but doesn't change the dropdown will get no Game Pass import. These should probably match — I'd suggest defaulting to "none" in the code as well, so Game Pass import is opt-in.

Inconsistent parameter passing

get_gamepass_catalog(plan, market) takes explicit args, which is clean. But get_product_details and get_owned_games_from_collections silently call get_resolved_xbox_gamepass_settings() internally to get the market. It would be more consistent (and testable) to pass market as a parameter to those functions too.

Otherwise this looks good — the plan map structure is clean, the UI fixes are solid, and the settings plumbing follows existing patterns well.

[ohadschn]

Naming convention

planInfo, collectionId, and subscriptionId use camelCase which breaks from the snake_case convention used throughout the rest of the codebase. Should be plan_info, collection_id, subscription_id.

Fixed, sorry about that - old habits die hard. Of course, Ruff could prevent such issues in the future: #46

KeyError risk on invalid plan

In get_gamepass_catalog, GAMEPASS_PLAN_MAP[plan] will raise a KeyError if plan isn't a valid key (and isn't "none"). Worth adding a .get() with a fallback or validation with an error message.

Technically that should never happen (unless the user uses devtools to change the option, or directly modify the DB, or something). So generally speaking, if it does happen, it's mostly likely a bug in the program which a developer would have to fix anyway. The exception would say something like KeyError: somePlanThatDoesntExist which combined with the stack trace would be just as good as "unexpected game pass plan". I added what you asked because it's just 2 more lines, but if any line that could fail on a bug would get its own special exception and turn to 3, you would have serious code bloat which is not worth the debuggability gain IMHO. Here something like Pyright (with enums) motioned in the above issue would help, delegating such checks to the type system (or ideally a statically type language altogether per #51).

Default plan mismatch

get_resolved_xbox_gamepass_settings() defaults to "ultimate" when no setting is stored, but the settings UI defaults the dropdown to "none". This means a user who never visits settings will silently get the Ultimate catalog imported, while a user who visits settings but doesn't change the dropdown will get no Game Pass import. These should probably match — I'd suggest defaulting to "none" in the code as well, so Game Pass import is opt-in.

Agreed, didn't realize there was such a state where no setting was stored.

Inconsistent parameter passing

get_gamepass_catalog(plan, market) takes explicit args, which is clean. But get_product_details and get_owned_games_from_collections silently call get_resolved_xbox_gamepass_settings() internally to get the market. It would be more consistent (and testable) to pass market as a parameter to those functions too.

Fair enough, fixed - I admit I didn't closely follow what calls what, maybe I'll take a stab at some refactoring to make the methods a bit shorter and easy to follow.

[ohadschn]

Oh and BTW, may I suggest separate, semantic, contextual comments (on the relevant lines of code)? instead of one large comment in the discussion? Easier to track and less likely for things to be missed IMHO (you fix one, mark as resolved, the commenter verified). There is a reason they exist after all :)