Add logger.warning for default and missing mapped values #174
Conversation
|
Please check https://antoniomrtz.github.io/SpotifyElectron/Git-Convention/ for naming convention for PR |
| @@ -81,6 +81,8 @@ def _set_attributes(self, config_section: str): | |||
| for key, value in self.config.items(config_section): | |||
| if value == "": | |||
| value = None | |||
| if value is None: | |||
There was a problem hiding this comment.
Unify if value is None: inside if value == "": in the same if statement. Use or
| @@ -91,7 +93,7 @@ def _load_architecture(self): | |||
| if not architecture_type: | |||
| architecture_type = DEFAULT_ARCHITECTURE | |||
| self.__setattr__(ARCHITECTURE_ENV_NAME, DEFAULT_ARCHITECTURE) | |||
There was a problem hiding this comment.
I think this line has no effect, because line self.__setattr__(ARCHITECTURE_ENV_NAME, architecture_type) is already loading the architecture name. Just delete it.
There was a problem hiding this comment.
you are taking about this line right?
architecture_type = DEFAULT_ARCHITECTURE
There was a problem hiding this comment.
or are talking about the whole if block ? @AntonioMrtz
There was a problem hiding this comment.
you are taking about this line right? architecture_type = DEFAULT_ARCHITECTURE
nope. Only self.__setattr__(ARCHITECTURE_ENV_NAME, DEFAULT_ARCHITECTURE) is redundant.
aarshgupta24
changed the title
|
@aarshgupta24 No need for prefix in PR name. Use as an example |
aarshgupta24
changed the title
|
@AntonioMrtz , sorry i am bit confused , you as per your review i will remove the redundant log statement that i added but after that no thing much is changed , so should i go through the code once more ? or everything is good and i should just move forward with only the change of info->warning? |
Remove the mentioned redudant line and unify both if statements into only 1. Then add the new |
| @@ -81,8 +81,6 @@ def _set_attributes(self, config_section: str): | |||
| for key, value in self.config.items(config_section): | |||
| if value == "": | |||
| value = None | |||
| if value is None: | |||
| properties_manager_logger.warning(f"Using None for {key} in {config_section}") | |||
There was a problem hiding this comment.
Unify this statement with the one above. Something like if value=="" or None. Then inside join both value=None and the new log message
|
@AntonioMrtz , im done with the changes please go through them and tell me if something else is needed |
| @@ -79,8 +79,9 @@ def _set_attributes(self, config_section: str): | |||
|
|
|||
| """ | |||
| for key, value in self.config.items(config_section): | |||
| if value == "": | |||
| if value == "" or None: | |||
There was a problem hiding this comment.
I think last comment might have confused you. In Python you have to either if value=="" or value is None instead. You can use also if value in ["",None]
There was a problem hiding this comment.
@AntonioMrtz , sorry for that i knew this still made an error.
| @@ -81,6 +81,7 @@ def _set_attributes(self, config_section: str): | |||
| for key, value in self.config.items(config_section): | |||
| if value == "": | |||
There was a problem hiding this comment.
As i said this has to be if value == "" or value is None
Description
added warning log where default value was being used
Commit type
refactor
Issue
Logging warning level if there is no mapping available and the default is being loaded.
Solution
logger.warningis added for default valueProposed Changes
-added logger.warning in
PropertiesManager-changed info log to warning log
Potential Impact
Help in checking for further problems where default value is being loaded.
Assigned
@AntonioMrtz