Skip to content

Conversation

@marcelveldt
Copy link
Member

Add type hints to the config controller functions to get config entry values.
This prevents having to do a lot of type casts everywhere when retrieving config values.

Copilot AI review requested due to automatic review settings November 14, 2025 21:03
@marcelveldt
Copy link
Member Author

@OzGav FYI - this helps your other PR(s)

Copilot finished reviewing on behalf of marcelveldt November 14, 2025 21:07
Copy link
Contributor

Copilot AI left a comment

Choose a reason for hiding this comment

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

Pull Request Overview

This PR adds type hints to config controller functions to eliminate the need for type casts when retrieving configuration values. The changes introduce overloaded function signatures with a return_type parameter for type inference, along with TypeVar-based type hints for better static type checking.

Key Changes

  • Added _ConfigValueT TypeVar bound to ConfigValueType for type inference in config getter functions
  • Added overloaded signatures to get_provider_config_value, get_player_config_value, get_core_config_value, and the get_raw_* methods
  • Removed explicit type casts throughout the codebase in favor of the new return_type parameter
  • Added comprehensive docstrings explaining the new return_type parameter

Reviewed Changes

Copilot reviewed 5 out of 5 changed files in this pull request and generated 4 comments.

Show a summary per file
File Description
music_assistant/controllers/config.py Core changes adding TypeVar definition, overload signatures for all config getter methods, and updated docstrings
music_assistant/providers/universal_group/player.py Removed cast and updated to use return_type parameter with positional argument syntax
music_assistant/providers/squeezelite/provider.py Removed cast from get_raw_player_config_value call and replaced cast with int() for explicit type conversion
music_assistant/providers/airplay/protocols/raop.py Removed explicit None default argument and updated to use return_type as keyword argument
music_assistant/controllers/music.py Removed cast and updated to use return_type as keyword argument

💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.

Comment on lines +253 to +259
"""
Return single configentry value for a provider.
:param instance_id: The provider instance ID.
:param key: The config key to retrieve.
:param return_type: Optional type hint for type inference (e.g., str, int, bool).
"""
Copy link

Copilot AI Nov 14, 2025

Choose a reason for hiding this comment

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

[nitpick] The return_type parameter is used purely for type inference and doesn't perform runtime type validation. This means if a caller specifies return_type=str but the actual config value is an int, the type checker will assume it's a str while the runtime value will be an int, potentially causing type safety issues.

Consider adding a note in the documentation to clarify that return_type is for static type checking only and callers are responsible for ensuring the specified type matches the actual config value type. Alternatively, consider adding runtime validation with isinstance() to catch type mismatches early.

Copilot uses AI. Check for mistakes.
self,
player_id: str,
key: str,
unpack_splitted_values: bool = False,
Copy link

Copilot AI Nov 14, 2025

Choose a reason for hiding this comment

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

The return_type parameter should be keyword-only (using *, before it) to be consistent with the other config getter methods (get_provider_config_value and get_core_config_value), which both use keyword-only parameters for return_type. This ensures API consistency and prevents potential errors from positional argument ordering.

Additionally, usage in this file should be updated to use keyword argument syntax: return_type=str instead of passing str positionally.

Suggested change
unpack_splitted_values: bool = False,
unpack_splitted_values: bool = False,
*,

Copilot uses AI. Check for mistakes.
"str",
await self.mass.config.get_player_config_value(child_player_id, CONF_HTTP_PROFILE),
http_profile = await self.mass.config.get_player_config_value(
child_player_id, CONF_HTTP_PROFILE, False, str
Copy link

Copilot AI Nov 14, 2025

Choose a reason for hiding this comment

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

The return_type parameter should be passed as a keyword argument (return_type=str) instead of a positional argument for consistency with similar usages elsewhere in the codebase (e.g., airplay/protocols/raop.py line 67) and to match the API design of other config getter methods.

Suggested change
child_player_id, CONF_HTTP_PROFILE, False, str
child_player_id, CONF_HTTP_PROFILE, False, return_type=str

Copilot uses AI. Check for mistakes.
Comment on lines +509 to +525
@overload
async def get_player_config_value(
self,
player_id: str,
key: str,
unpack_splitted_values: bool = ...,
return_type: type[_ConfigValueT] = ...,
) -> _ConfigValueT: ...

@overload
async def get_player_config_value(
self,
player_id: str,
key: str,
unpack_splitted_values: bool = ...,
return_type: None = ...,
) -> ConfigValueType | tuple[str, ...] | list[tuple[str, ...]]: ...
Copy link

Copilot AI Nov 14, 2025

Choose a reason for hiding this comment

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

The overloads don't correctly handle the interaction between unpack_splitted_values and return_type. When unpack_splitted_values=True, the function always returns tuple[str, ...] | list[tuple[str, ...]] (from line 545) regardless of the return_type parameter. The overloads should be structured as:

  1. When unpack_splitted_values=True → return tuple[str, ...] | list[tuple[str, ...]]
  2. When unpack_splitted_values=False and return_type=type[T] → return T
  3. When unpack_splitted_values=False and return_type=None → return ConfigValueType

This requires using Literal[True] and Literal[False] in the overloads to properly discriminate the return type based on the unpack_splitted_values parameter.

Copilot uses AI. Check for mistakes.
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants