Replaces prints with logging functions #156
Conversation
jcemelanda
force-pushed
the
master
branch
2 times, most recently
from
557b546
to
6b105be
Compare
There was a problem hiding this comment.
It looks really good @jcemelanda! Many many thanks. I added some minor inline suggestions, but the main point is: wouldn't it be interesting to have a named logging object so we could configure it later (for example, logging to a file or to stdout)? Then all files would import from something like serenata_toolbox.logging…
| @@ -33,7 +34,7 @@ def fetch(self, start_date, end_date): | |||
| records = [] | |||
| for two_months_range in self._generate_ranges(start_date, end_date): | |||
| if os.environ.get('DEBUG') == '1': | |||
| print(two_months_range) | |||
| logging.debug(two_months_range) | |||
There was a problem hiding this comment.
In this case, as the logging level is debug, we can get rid of the if.
| @@ -36,7 +37,7 @@ def fetch(self, deputies, start_date, end_date): | |||
| :param date_end: (str) date in the format dd/mm/yyyy | |||
| """ | |||
| if os.environ.get('DEBUG') == '1': | |||
| print("Fetching data for {} deputies from {} -> {}".format(len(deputies), start_date, end_date)) | |||
| logging.debug("Fetching data for {} deputies from {} -> {}".format(len(deputies), start_date, end_date)) | |||
There was a problem hiding this comment.
In this case, as the logging level is debug, we can get rid of the if
| @@ -58,7 +59,7 @@ def _all_presences(self, deputies, start_date, end_date): | |||
| error_count = 0 | |||
| for i, deputy in deputies.iterrows(): | |||
| if os.environ.get('DEBUG') == '1': | |||
| print(i, deputy.congressperson_name, deputy.congressperson_document) | |||
| logging.debug(i, deputy.congressperson_name, deputy.congressperson_document) | |||
There was a problem hiding this comment.
In this case, as the logging level is debug, we can get rid of the if.
| @@ -72,33 +73,33 @@ def _all_presences(self, deputies, start_date, end_date): | |||
| time.sleep(self.sleep_interval) | |||
|
|
|||
| if os.environ.get('DEBUG') == '1': | |||
| print("\nErrored fetching", error_count, "deputy presences") | |||
| logging.debug("\nErrored fetching", error_count, "deputy presences") | |||
There was a problem hiding this comment.
In this case, as the logging level is debug, we can get rid of the if.
| # 500 seems to be the error code for "no data found for the | ||
| # params provided" | ||
| if err.code == 500: | ||
| print("SKIP") | ||
| logging.error("SKIP") |
There was a problem hiding this comment.
Outputting SKIP IMHO is mostly an INFO, what do you think?
There was a problem hiding this comment.
And we could be more explicit about what we're skipping.
| else: | ||
| print("FAIL") | ||
| logging.error("FAIL") |
There was a problem hiding this comment.
We could be more explicit about what just failed.
| else: | ||
| print("FAIL") | ||
| logging.error("FAIL") |
There was a problem hiding this comment.
And we could be more explicit about what just failed.
| @@ -35,7 +36,7 @@ def fetch(self, pivot, session_dates): | |||
| def _all_start_times(self, pivot, session_dates): | |||
| for date in session_dates: | |||
| if os.environ.get('DEBUG') == '1': | |||
| print(date.strftime("%d/%m/%Y")) | |||
| logging.debug(date.strftime("%d/%m/%Y")) | |||
There was a problem hiding this comment.
In this case, as the logging level is debug, we can get rid of the if.
|
Great job @jcemelanda ! |
|
Thanks! I'll go for it A.S.A.P.
|
Use logs instead of print (extends #156)
|
Closed by #163 |
This pullrequest replaces the print calls throughout the code with the logging module.
I believe this review could be done by anyone with deep enough understanding of the code to know if the logging methods called are really the best suited ones for each place.
I tried to define the log level of each call based on the code around each print call. Usually I used logging.info, errors when dealing with errors and exceptions and warnings when I saw the message was a warning message.
Fixes #25