Pavel_Los_lospawel@yandex.ru by paxalos · Pull Request #28 · epam-python-courses-7-bsu/FinalTaskRssParser

paxalos · 2019-11-20T21:54:00Z

5 iterations

HenadziStantchik · 2019-11-21T10:11:24Z

Please make your PR name correspond to the one specified in our README.md

HenadziStantchik · 2019-11-21T14:54:09Z

Here is some feedback on your app work:

User should be able to use --version without specifying RSS url
Your app does not work on Ubuntu OS:

paxalos · 2019-11-21T15:10:05Z

Doesn't work, because you need to install mysql, then database final_task_database and table nes_cache like in readme

HenadziStantchik · 2019-11-21T15:15:06Z

So the user should install MySQL client, create specific database schema and create specific table inside by himself just to be able to run your application?

How does this work with setuptools?

paxalos · 2019-11-21T15:20:27Z

Yes

paxalos · 2019-11-21T15:34:59Z

I used database, because it will be necessary in 6 iteration. But now I think that don't have time to do it, that is why I can change datebase storage to store data in simple file. The database doesn't depend on setuptools

HenadziStantchik · 2019-11-21T15:52:51Z

It is just not a good practice then. An average user (who does not know MySQL at all) will not be able to use your application.

It is up to you to decide what you are going to do, you still have 4 full days until the deadline.

If you really using database for Iteration 6 then it is ok, but make sure that steps of schema and table creation will be automated, so the user will just need to write 1-2 lines in the console to get things going on Iteration 6. You also can try some libs that use mysql, but actually do not require MySQL server installed and operational.

If you are not going to do the 6th iteration the it is of course preferable to simplify things for user: either try to automate it using bash scripts or just switching to a different storage method.

Or you can leave it as is. Of course it is not a good practice to make user manually setup the database, but we will surely not deny/fail your hard work just because of this issue.

P.S. I will test your app today a bit later

HenadziStantchik · 2019-11-21T18:57:40Z

Please check my comment on your README.md

HenadziStantchik · 2019-11-21T19:56:54Z

User should be able to user --version without specifying rss url
--date does not seem to work. I get "No news by this datefor any date I specify. ( while the tablenews_cache` contains news data)
Currently you do not have any exception handling on actions with the databse. If anything goes wrong user gets hyge traceback to stdout, which shoiuld not happen

…move parsing arguments in separate function, --date and --version can be used without source, fix bugs with writing and reading database. Some other little changes

HenadziStantchik · 2019-11-22T10:25:17Z

I get constant warnings about incorrect date format. It is better either suppress them(not that good option) or fix your datetime->date conversion(preferable option)
--date does not work together with --json
HTML conversion does not work: if user specifies folder then IsADirectoryError is thrown with traceback in stdout. If user specifies file he gets Incorrect html filepath (Ubuntu OS)

Argument that user specifies for conversion to another format should be a directory, not a specific file. Moreover right now if you specify a directory instead of filename it will throw an exception with traceback in stdout

FB2 conversion does not work:

Keep in mind that conversion, --colorize and --json should work with --date
(optional) please rethink your color schema design for text background:

--date, redo --to-fb2 and --to-html: now the argument is directory and file news.fb2 or news.html are created in this directory. Some other little changes

HenadziStantchik · 2019-11-22T12:51:59Z

There are some redundant symbols in fb2 and html converted files:

…d html formats

HenadziStantchik · 2019-11-23T12:41:54Z

I did not discover issues with Iteration 5 functionality. Still be sure to test it yourself, in case I missed something.

HenadziStantchik · 2019-12-01T16:25:37Z

+        news_list.append({'Feed':
+                              html.unescape(parsed_rss['feed']['title']),
+                          'Title':
+                              html.unescape(parsed_rss['entries'][index]['title']),
+                          'Date':
+                              str(date_parser.parse(parsed_rss['entries'][index]['published'])).split(" ")[0],
+                          'Link':
+                              parsed_rss['entries'][index]['link'],
+                          'Image description':
+                              html.unescape(get_image_description(parsed_rss['entries'][index]['summary'])),
+                          'New description':
+                              html.unescape(get_new_description(parsed_rss['entries'][index]['summary'])),
+                          'Image links':
+                              [content['url'] for content in parsed_rss['entries'][index]['media_content']]})


For multiline dictionary declarations it is better to use this codestyle:

news_dict = { "key`": value1, .... }

HenadziStantchik · 2019-12-01T16:26:54Z

+from database_functions import get_news_list_by_date, write_news_to_database
+from parse_rss_functions import get_news_list
+from custom_exceptions import NoInternet, IncorrectURL, IncorrectFilePath, DatabaseConnectionError
+from print_functions import print_news_colorize, print_news_JSON_colorize, print_news, print_news_JSON


Here and everywhere else:
If you are using all functions inside imported module it is preferable to use just import

HenadziStantchik · 2019-12-01T16:41:40Z

Here is some feedback on your application:
Iteration 1:
Everything works fine.
Iteration 2:
Everything installs and works, but only if you install libs specified in requirements.txt first, because not all of them are specified in the dependencies in setup.py
Iteration 3:
News caching does not work for news using Cyrillic symbols. (for example news.tut.by/rss)
Iteration 4:
Everything works fine.
Iteration 5:
Everything works fine.

Commit messages are informative enough and correspond to the recommended commit message style.
Tests are almost nonexistent and coverage percentage is not enough.
Code is mostly readable, but there are some large functions containing too much if/else branching, which lower readability. The code absolutely corresponds to the PEP8 guidelines.

HenadziStantchik · 2019-12-01T17:52:53Z

+usage: rss_reader.py [-h] [--source SOURCE] [--version] [--json] [--verbose]
+[--limit LIMIT] [--date DATE]
+
+Pure Python command-line RSS reader.
+
+optional arguments:
+
+-h, --help            show this help message and exit
+--source SOURCE            RSS URL
+--version            Print version info
+--json            Print result as JSON in stdout
+--verbose            Outputs verbose status messages
+--limit LIMIT            Limit news topics if this parameter provided
+--date DATE            News from the specified day will be printed out. Format: YYYYMMDD
+It is mandatory to specify date or/and time.
+If both are specified, then news will be searched by date and by source.


And what about Iteration 4 options?

paxalos added 5 commits November 21, 2019 00:18

Complete first iteration with tests

03a00a2

Complete second iteration

3c71841

Complete third iteration

5fb38b5

Complete fourth iteration with README

63c3135

Complete fifth iteration

0747d20

paxalos changed the title ~~Lospawel@yandex.ru Please Review~~ Pavel_Los_Lospawel@yandex.ru Please Review Nov 21, 2019

paxalos changed the title ~~Pavel_Los_Lospawel@yandex.ru Please Review~~ Pavel_Los_lospawel@yandex.ru-PLEASE REVIEW Nov 21, 2019