-
-
Notifications
You must be signed in to change notification settings - Fork 24
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Change to use getlogger #36
base: main
Are you sure you want to change the base?
Conversation
Fix logging
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Hi @johro897 - thanks a lot for your contribution and taking the time to do this. This is definitely a helpful change and I am happy to merge it. However, in this state, these changes introduce several changes in code style (e.g. f-strings, 80 chars line limit), messaging (e.g. removing self._name, using Couldnt
instead of Couldn't
) and flow logic with impact in readability (some if-statement changes).
I've added comments where I saw these points. However, maybe an easier approach is that, given your description, there should only be exactly 2 types of changes:
- Add
_LOGGER = logging.getLogger(__name__)
where the module constants are defined. - Replace all instances of
logging.xxx
with_LOGGER.xxx
There should be no other changes regarding messaging or logic.
Would you be open to making these changes so we can merge your pull request?
) | ||
) | ||
if date_value: | ||
message = 'No Oura data found for '+ date_name_title +' ('+ original_date +'). Fetching '+ date_value + 'instead.' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This has an issue because there's no space before instead, so the date would merge. Also space before and after spaces are desirable for readability.
To solve for all this, let's change to f-strings here and 258.
) | ||
) | ||
if date_value: | ||
message = 'No Oura data found for '+ date_name_title +' ('+ original_date +'). Fetching '+ date_value + 'instead.' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This comment is missing self._name
which might be helpful for debugging.
@@ -362,8 +359,7 @@ def parse_sensor_data(self, oura_data, data_param='data', day_param='day'): | |||
Oura sensor data for that given day. | |||
""" | |||
if not oura_data or data_param not in oura_data: | |||
logging.error( | |||
f'Oura ({self._name}): Couldn\'t fetch data for Oura ring sensor.') | |||
_LOGGER.error('Couldnt fetch data for Oura ring sensor.') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Couldn't
- to avoid probelsm with the single quotes, you can escape it 'Couldn't ...'
Aso, this is missing self._name
. It might be worth just leaving Oura ({self._name}) on all of them even if this is now done by _LOGGER which will include the module name.
message = 'No Oura data found for '+ date_name_title +' ('+ original_date +'). Fetching '+ date_value + 'instead.' | ||
else: | ||
message = 'Unable to find suitable backfill date. No data available.' | ||
_LOGGER.warning(message) | ||
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ditto from before: change to f-strings and keep Oura ({self._name}.
message = 'No Oura data found for '+ date_name_title +' ('+ original_date +'). Fetching '+ date_value + 'instead.' | ||
else: | ||
message = 'Unable to find suitable backfill date. No data available.' | ||
_LOGGER.warning(message) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
nit: to keep consistent with the other file (e.g.sensor_base_dated lines 258-260) add a break line before _LOGGER.warning()
@@ -6,6 +6,7 @@ | |||
from homeassistant.helpers import entity | |||
from . import api | |||
|
|||
_LOGGER = logging.getLogger(__name__) |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Actually, _LOGGER is not used in this file. I think a better change would be removing line 9 and import logging
on line 3 altogether. My bad.
logging.info( | ||
f'Oura ({self._name}): ' + | ||
'Unknown day name `{date_name}`, using yesterday.') | ||
_LOGGER.info('Unknown day name ' + date_name + 'using yesterday.') |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Change to f-strings and keep self._name. The first change would ensure higher consistency and readability. The second would help in troubleshooting when multiple Oura sensors are added.
) | ||
) | ||
if date_value: | ||
message = 'No Oura data found for '+ date_name_title +' ('+ original_date +'). Fetching '+ date_value + 'instead.' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This exceeds 80 chars. Break it into lines of max 80 chars. That's why the original is broken down into concatenated strings.
f'Unable to find suitable backfill date. No data available.' | ||
) | ||
) | ||
if date_value: |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we keep the original if logic structure?
) | ||
) | ||
if date_value: | ||
message = 'No Oura data found for '+ date_name_title +' ('+ original_date +'). Fetching '+ date_value + 'instead.' |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Ditto: change to f-strings, keep lines below 80 chars and keep Oura ({self._name})
update to use getlogger in order to remove the [root] message in logs