Anna_Gonchar_raphaelkyzy@gmail.com#16
Conversation
| import argparse | ||
|
|
||
|
|
||
| class ArgReader: |
There was a problem hiding this comment.
Why do you need a wrapping class for parser? You could just make a function just like init() method here and return parser object
|
Here is some feedback on your application work:
|
|
AnnaPotter
changed the title
|
Iteration 1 does not seem to have issues anymore. Proceed to the next Iteration |
HenadziStantchik
changed the title
AnnaPotter
changed the title
HenadziStantchik
changed the title
|
README.md will be fixed in the next commit. |
AnnaPotter
changed the title
| # get valid limit argument | ||
| # if not initialize limit argument | ||
| if not check_limit_arg(com_line_args, logger): | ||
| limit = len(news_collection["entries"]) |
There was a problem hiding this comment.
In that case, you may want to set limit to None and refactor your logic below.
| limit = com_line_args.limit | ||
|
|
||
| if len(news_collection["entries"]) < limit: | ||
| logger.warning("The number of news is less than the value of the argument limit.") |
There was a problem hiding this comment.
To my mind, it's not the warning that user may want to see. For example, we have only 3 news and the limit is 5. It's completly ok. You could notify user, available news > chosen limit. But this is optional
There was a problem hiding this comment.
This warning only for me, users can see only errors or higher level logs.
dzhigailo
changed the title
AnnaPotter
changed the title
|
HenadziStantchik
changed the title
| Functions: | ||
| create_logger(com_line_args) -> logger | ||
| get_com_line_args() -> com_line_args | ||
| get_news(command_line_args, logger) -> news_collection | ||
| print_news_stdout(news_collection) -> None | ||
| print_news_json(news_collection) -> None | ||
| print_news(news_collection, com_line_args, logger) -> None | ||
| print_cache_news(news_collection, logger) -> None | ||
| print_cache_news_json(news_collection, logger) -> None | ||
| convert_date(date_str, logger) -> str_date | ||
| clean_str(string) -> clean_string """ |
There was a problem hiding this comment.
Here and below: there is actually no need to write function list in module docstring.
| pdf.set_font('Arial', size=12) | ||
| pdf.set_text_color(0, 255, 0) | ||
| pdf.ln(10) | ||
| pdf.multi_cell(0, 10, align="C", txt="News") | ||
| pdf.set_text_color(0, 0, 255) | ||
| pdf.write(10, "Feed title: ") | ||
| pdf.set_text_color(0, 0, 0) | ||
| pdf.multi_cell(0, 10, txt=f" {news.feed_title}") | ||
| pdf.set_text_color(0, 0, 255) | ||
| pdf.write(10, "News title: ") | ||
| pdf.set_text_color(0, 0, 0) | ||
| pdf.multi_cell(0, 10, txt=f"{news.title}") | ||
| pdf.set_text_color(0, 0, 255) | ||
| pdf.write(10, "News publication date: ") | ||
| pdf.set_text_color(0, 0, 0) | ||
| pdf.multi_cell(0, 10, txt=f"{news.date}") | ||
| pdf.set_text_color(0, 0, 255) | ||
| pdf.write(10, "Summary: ") | ||
| pdf.set_text_color(0, 0, 0) | ||
| pdf.multi_cell(0, 10, txt=f"{news.summary}") | ||
| pdf.set_text_color(0, 0, 255) | ||
| pdf.write(10, "News link: ") | ||
| pdf.set_text_color(0, 0, 0) |
There was a problem hiding this comment.
It is better to think of a way to simplify this.
|
Here is feedback on your application: Tests are decently written. Coverage percentage is high enough. |
|
Thanks a lot for review. A little annoying for the third iteration, because I checked. But now it really does not work... Thanks again) |
| if source: | ||
| for hash_date_key in news_dict: | ||
| if date in hash_date_key: | ||
| if hash_date_key.split()[1] == date.partition(' ')[0]: |
There was a problem hiding this comment.
This line (and the same line below) breaks your date search algorithm for days of month that start from 0, e.g. 01,02,03.
For example for specified --date 20191203 in this function:
date = 3 Dec 2019
hash_date_key = 'Tue, 03 Dec 2019 07:11:16 -0500'
date in hash_date_key -> True
BUT:
hash_date_key.split()[1] -> '03'
and
date.partition(' ')[0] -> '3'
I actually noted that your Iteration 3 worked before final review.
After a bit of investigation, check my comment about |
|
You are right, it was these two lines that broke the whole iteration. It’s just that the dates came in the format (Publication date: Wed, 5 Feb 2014 09:00:00 -0500) on my news feed and my previous version of the function with parameter --date 20190205 / 2019025, for example, did not work correctly. It displayed either the lack of news, or the news from 20190215/20190225. I tried to fix this bug, but I did not think through it thoroughly. Next time I will be more attentive. |
3 participants
No description provided.