Victoria_Kondrat'eva_sam.kondrateva@gmail.com#14
Conversation
adds unittests
|
Here is some feedback on your application work:
5 (optional). It would be nice if you add some more logging. |
HenadziStantchik
changed the title
| bs = BeautifulSoup(news['summary'], 'html.parser') | ||
| img = bs.find_all('img') | ||
| image_data = 'No image' | ||
| if len(img) > 0: |
There was a problem hiding this comment.
Such check should look: if not img:
| dictionary_of_news_data = self.make_news_data(news) | ||
| except Exception as ex: | ||
| self.log("Error processing news: {} {}".format(type(ex), ex)) | ||
| continue # continues to process the following news |
There was a problem hiding this comment.
You dont need continue in here. After handling of an excetion, cycle will continue anyway
Victoria-Sam
changed the title
| self.logger.warning("Warning! Caching file is not found!") | ||
| self.cache_news = {} | ||
|
|
||
| def caching(self, all_news, url): |
There was a problem hiding this comment.
Here and below: please use more informative method names
|
HenadziStantchik
changed the title
Victoria-Sam
changed the title
|
Pdf conversion seems bugged:
Keep in mind that user should be able to specify the path where converted file should be saved. |
HenadziStantchik
changed the title
| import sys | ||
| sys.path.append('rss_reader') | ||
| from setuptools import setup, find_packages | ||
| from rss_reader import rss_reader |
There was a problem hiding this comment.
This will not work on clean machine, if there are no libs from requirements.txt installed.
There was a problem hiding this comment.
I tried running on a clean machine. When libraries is installed, just start it as rss_reader.rss_reader.
Example:
python -m rss_reader.rss_reader "http://news.yahoo.com/rss/" --limit 2 --verbose
There was a problem hiding this comment.
When libraries is installed
Yes, when libraries are installed. The idea of second iteration is that your app will download required libs itself, without you doing pip install -r requirements.txt
python -m rss_reader.rss_reader "http://news.yahoo.com/rss/" --limit 2 --verbose
Your app works perfectly while using python rss_reader..., But the idea of second iteration is to install a CLI utility, that will allow you to run your app like this:
rss-reader <rss_source> <options>
Without manually installing required libs.
|
|
||
| setup( | ||
| name='rss_reader', | ||
| version=rss_reader.VERSION, |
There was a problem hiding this comment.
In this specific case it is better either hardcode this value or import in from some file.
| description='RSS Reader', | ||
| entry_points={'console_scripts': ['Rss-Reader = rss_reader.rss_reader:main']}, | ||
| keywords="rss-reader", | ||
| python_requires='>=3.7', |
|
Here is feedback on your application: Tests are ok, but the coverage is barely enough. |
|
|
||
| def get_output_function(converter): | ||
| """Output factory""" | ||
| if converter == 'json': |
There was a problem hiding this comment.
Instead if multiple if statements you could create dictionary and get needed handler by key, e.g
{
'json': json_handler,
....
}
3 participants
Five iterations.