AntonioMrtz / SpotifyElectron

Spotify Electron is a cross-platform music streaming desktop app made with Electron-React frontend and Python-FastAPI-AWS Backend. The goal is to reproduce the original Spotify functionality while adding new ones like uploading songs.
https://antoniomrtz.github.io/SpotifyElectron_Web/
Other
37 stars 9 forks source link

Warning logs for default selections & mappers #165

Closed AntonioMrtz closed 1 month ago

AntonioMrtz commented 1 month ago

Description

Add logger.warning to events that can lead to future problems. Providers or mappers are a clear example. Log in warning level if theres no mapping available and the default is being loaded.

Context

Dynamic load such as mappers and providers cant have an assigned value and will load a default value.

How to do it

Check that every mapper/provider is logging a warning if the default value is being used. Also check PropertiesManager, it has some default selections for architecture.

Testing

AntonioMrtz commented 1 month ago

Warning logs for default selections & mappers

aarshgupta24 commented 1 month ago

@AntonioMrtz , i had two doubts 1 - playlist_collection_provider , here there is no need of logging right?

2- song_service_provider :

def _get_song_service(self) -> ModuleType:
    architecture_type = getattr(PropertiesManager, ARCHITECTURE_ENV_NAME)
    if architecture_type not in self.song_services:
        self.logger.warning(
            f"Missing mapping for architecture type:  {architecture_type}."
        )
    import_module = importlib.import_module(
        MODULE_PREFIX_NAME + self.song_services[architecture_type]
    )
    self.logger.info(f"Song service MODULE selected : {architecture_type}")
    return import_module

Does this seems correct im kind of confused as we are already setting default value in properties_manager, and also if this is correct then i have to return default module or not?

AntonioMrtz commented 1 month ago

@AntonioMrtz , i had two doubts 1 - playlist_collection_provider , here there is no need of logging right?

2- song_service_provider :

def _get_song_service(self) -> ModuleType:
    architecture_type = getattr(PropertiesManager, ARCHITECTURE_ENV_NAME)
    if architecture_type not in self.song_services:
        self.logger.warning(
            f"Missing mapping for architecture type:  {architecture_type}."
        )
    import_module = importlib.import_module(
        MODULE_PREFIX_NAME + self.song_services[architecture_type]
    )
    self.logger.info(f"Song service MODULE selected : {architecture_type}")
    return import_module

Does this seems correct im kind of confused as we are already setting default value in properties_manager, and also if this is correct then i have to return default module or not?

Hi @aarshgupta24 .

  1. Theres no need really because there isnt any default selection of the argument is not provided.
  2. As you say, default selection is made on properties_manager. The log should be in properties_manager, but in this case I think it already exists. No need to log everything extra on _get_song_service
aarshgupta24 commented 1 month ago

@AntonioMrtz , hey i have gone through the entire code and only found changes in propertiesmanager , else i didn't found any other changes. so should i move forward with pull request?

AntonioMrtz commented 1 month ago

@aarshgupta24 , yeah, I will check it out as soon as possible