Ilya_Khonenko_honenkoi@gmail.com by kingofmidas · Pull Request #7 · epam-python-courses-7-bsu/FinalTaskRssParser

kingofmidas · 2019-11-05T15:57:48Z

[Iteration 4] Format converter. Without unit tests.

HenadziStantchik · 2019-11-05T20:03:18Z

Please change commit messages or squash commits. Unfortunatelly right now half of the commit messages are really uninforamtive.

HenadziStantchik · 2019-11-05T20:39:12Z

Here is some feedback of your application work:

Encoding problem (check your own example in README.md)
You cannot get app version without specifying some RSS resource (e.g. python rss_parser.py --version does not work)
In command line it would be nice to see at what place in the description exactly image was. (see the example in FinalTask.md)
When using --json option your app prints BOTH json and normal versions.
When reading news from cache using --date news are not separated by empty lines or any other separators. Looks a bit messy.
When using --date with no internet connection script does not work even if I have cached news file for specified date. It is kind of eliminates purpose of using cache if you cannot access it without internet connection.
HTML conversion does not work with --verbose argument
PDF conversion does not work at all for me. Even got traceback and this should not happen at all.

HTML conversion does not work with --date argument
--date argument does not get limited by --limit argument.
It would be nice if resulting HTML files names were a bit more specific. 33.html is hardly tells user anything useful

kingofmidas · 2019-11-09T13:56:10Z

Here is some feedback of your application work:

Encoding problem (check your own example in README.md)

You cannot get app version without specifying some RSS resource (e.g. python rss_parser.py --version does not work)

In command line it would be nice to see at what place in the description exactly image was. (see the example in FinalTask.md)

When using --json option your app prints BOTH json and normal versions.

When reading news from cache using --date news are not separated by empty lines or any other separators. Looks a bit messy.

When using --date with no internet connection script does not work even if I have cached news file for specified date. It is kind of eliminates purpose of using cache if you cannot access it without internet connection.

HTML conversion does not work with --verbose argument

PDF conversion does not work at all for me. Even got traceback and this should not happen at all.

HTML conversion does not work with --date argument

--date argument does not get limited by --limit argument.

It would be nice if resulting HTML files names were a bit more specific. 33.html is hardly tells user anything useful

Fixed

…into Html or Pdf format

HenadziStantchik · 2019-11-10T22:11:06Z

Good job, everything works fine.
Resolve these small issues in comments and proceed to Iteration 5.

… create web server on flask with postgresql. create dockerfile and docker-compose for docker containers.

HenadziStantchik · 2019-11-22T11:52:22Z

There is SQL-related error on the server. Looks like it occurs when there is already such news entry in the db:

--json output is not colorized
Please make colorization from --colorize more complex
Please add some sort of additional functionality to the server, for example client authorization, make the server have list of RSS sources and fetch news from them form time to time and the clients can add/remove rss source and etc.

HenadziStantchik · 2019-11-22T12:56:41Z

Please make parser.log not read-only

HenadziStantchik · 2019-11-26T12:15:21Z

+DEBUG    [2019-11-25 21:10:55,763] http://127.0.0.1:5000 "GET /version/ HTTP/1.1" 200 3
+INFO     [2019-11-25 21:10:57,182] Website is working
+DEBUG    [2019-11-25 21:10:57,189] Starting new HTTP connection (1): 127.0.0.1:5000
+DEBUG    [2019-11-25 21:11:41,580] http://127.0.0.1:5000 "GET /print/ HTTP/1.1" 200 10160


Logging files should not be checked in to GIT

HenadziStantchik · 2019-11-26T12:18:14Z

+            if (index == limit):
+                break


It is better to use slices instead of comparing index with limit on each iteration

Same for other occurrences.

HenadziStantchik · 2019-11-26T12:23:22Z

+    finally:
+        con.close()


This may potentially cause UnboundLocalError when variable referenced before assignment.
This is possible in case we will fail to connect to DB for some reason.

It is better to do as stated in docs:

con = psycopg2.connect.... try: ... finally: con.close()

Same for another similar situation in news_parser file.

HenadziStantchik · 2019-11-26T13:39:56Z

Here is feedback on your application work:
Iteration 1 functionality:
Apart from minor encoding problems when using --json option, everything works fine.
Iteration 2:
Builds fine.
Iteration 3:
Everything works fine, but it would be nice to have a bit more info when printing cached news: date and etc. Also it would be nice if you had implemented search in cache not just by date, but by the source too (optional)
Iteration 4:
Everything works fine.
Iteration 5:
Everything works fine.
Iteration 6:
Everything works fine.

Overall aside from couple mishaps the code looks decent, commit messages informative and correspond to the recommended commit message style. Codestyle mostly corresponds to the PEP8 guidelines. Code is readable and clear. The application is decently structured.

dzhigailo · 2019-11-27T19:32:43Z

+
+
+if __name__ == '__main__':
+    app.run(debug=True)


Debug should not be True

dzhigailo · 2019-11-27T19:37:51Z

+    links: 
+      - localhost
+
+  localhost:


Why you database service is called localhost? It may cause misunderstanding

after testing forgot to change