Vlad_Protosevich_protosevich2001@gmail.com by protosevichVlad · Pull Request #9 · epam-python-courses-7-bsu/FinalTaskRssParser

protosevichVlad · 2019-11-05T20:08:18Z

Iteration 3
Add README and bug fixes

HenadziStantchik · 2019-11-05T23:15:34Z

Here is feedback about your script work:

You cannot get the app version without specifying source RSS (e.g. python rss_reader.py --version does not work)
You have encoding problem:

While you are parsing news correctly, it would be better if you at least numerate links to the images. You got them numerated in news feed, but in links they are not. Or for the future implementations it would be better group them like this:

Title: ...
Date: ...
Link: ...
Description: ...
Links:
[1] - image_1_url
[2] - image_2_url
...

This way you could awoid potential mistakes in the future convertations to other formats.

--verbose option does not work.
If --limit was not specified by user then you should print all available feed.
Your script freezes if --limit equals a certain number.
(A hint: try different large --limit and see what happens)
--limit does not work on number of news when using --date
If I use reader with --limit 5 and then with --limit 1 your script will save only 1 piece of news to the cache.

upd: please be sure to use labels in the future. Check slack for more info.

protosevichVlad · 2019-11-07T22:09:26Z

Iteration 3

HenadziStantchik · 2019-11-08T13:00:29Z

I am getting ModuleNotFoundError by trying to launch your app without using distribution tools. Please fix it.

HenadziStantchik · 2019-11-08T13:12:21Z

Here is feedback about your script work:

You cannot get the app version without specifying source RSS (e.g. python rss_reader.py --version does not work)

You have encoding problem:

While you are parsing news correctly, it would be better if you at least numerate links to the images. You got them numerated in news feed, but in links they are not. Or for the future implementations it would be better group them like this:
Title: ...
Date: ...
Link: ...
Description: ...
Links:
[1] - image_1_url
[2] - image_2_url
...
This way you could awoid potential mistakes in the future convertations to other formats.

--verbose option does not work.

If --limit was not specified by user then you should print all available feed.

Your script freezes if --limit equals a certain number.
(A hint: try different large --limit and see what happens)

--limit does not work on number of news when using --date

If I use reader with --limit 5 and then with --limit 1 your script will save only 1 piece of news to the cache.

upd: please be sure to use labels in the future. Check slack for more info.

Also please respond in comments what of these points did you fix.

protosevichVlad · 2019-11-08T18:03:22Z

Here is feedback about your script work:

You cannot get the app version without specifying source RSS (e.g. python rss_reader.py --version does not work)

You have encoding problem:

While you are parsing news correctly, it would be better if you at least numerate links to the images. You got them numerated in news feed, but in links they are not. Or for the future implementations it would be better group them like this:
Title: ...
Date: ...
Link: ...
Description: ...
Links:
[1] - image_1_url
[2] - image_2_url
...
This way you could awoid potential mistakes in the future convertations to other formats.

--verbose option does not work.

If --limit was not specified by user then you should print all available feed.

Your script freezes if --limit equals a certain number.
(A hint: try different large --limit and see what happens)

--limit does not work on number of news when using --date

If I use reader with --limit 5 and then with --limit 1 your script will save only 1 piece of news to the cache.

upd: please be sure to use labels in the future. Check slack for more info.
Also please respond in comments what of these points did you fix.

--version - fixed
encoding problem - fixed
--verbose - fixed
--limit - fixed

HenadziStantchik · 2019-11-08T21:15:14Z

Here is some feedback on your application:

1 (optional). It would be great to add some more logging to printong functions.

--date argument stopped working completely. instead I get FileNotFoundError with traceback to stdout.

Please address existing issues

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

2. HTML-convertation has encoding problems:

Sometimes there are empty lines in the image url list:

Please ensure that your app works with RSS feed urls other than yahoo.
For any other RSS url I tried I got an ERROR: HTTP STATUS CODE 301

Also please describe the issues you fixed either in the commit message or in the comments here

protosevichVlad · 2019-11-18T22:49:17Z

Sometimes there are empty lines in the image url list:

Please ensure that your app works with RSS feed urls other than yahoo.
For any other RSS url I tried I got an ERROR: HTTP STATUS CODE 301

Fixed

protosevichVlad · 2019-11-18T22:55:25Z

HTML-convertation has encoding problems:

If you will see symbols, take screenshots these symbols in cmd(terminal) or site

HenadziStantchik · 2019-11-19T11:52:18Z

HTML-convertation has encoding problems:

If you will see symbols, take screenshots these symbols in cmd(terminal) or site

Problem still persists

protosevichVlad · 2019-11-19T20:06:11Z

The problem is the symbol '. After checking in different browsers and on different OS the problem was not noticed. If you have a problem this time, please specify the browser through which you check it.

HenadziStantchik · 2019-11-20T11:23:54Z

The problem is the symbol '. After checking in different browsers and on different OS the problem was not noticed. If you have a problem this time, please specify the browser through which you check it.

The issue disappeared

protosevichVlad · 2019-11-23T11:20:29Z

Iteration 5. Colorize should work in pdf and html?

dzhigailo · 2019-11-23T13:25:15Z

+            raise RssReaderException.RssReaderException('How work with application?\nEnter in command line: rss-reader -h')
+
+        if args.version:
+            result = f'RSS reader version {__init__.VERSION}'


It's not a good practive to use init file like that. You can define it here, in this module or make a file VERSION.txt, for example, that will contain the version of your application

HenadziStantchik · 2019-11-28T12:45:46Z

+    try:
+        data = feedparser.parse(url)
+        try:
+            if data.status == 200:


It is better to avoid using status for checking if we received feed or not.

data here has type FeedParserDict, which is inherited from dict, so it is a dictionary, which has keys and values.
Neither FeedParserDict nor dict have attribute status, so theoretically you should always get an AttributeError here.
BUT FeedParserDict class has its magic method __getattribute__ redefined (note: getattribute is called every time you try to access some object attribute using .) in a way so if there is no such attribute (for example status), it will try to look in this dictionary keys, using __getitem__ (note: getitem is called when you try to acccess some item in collection by key or by index: data["status"]). So there is no attribute status, but status is a key in data dictionary, so it is still luckily works.

In short, using status here is pretty volatile and may break your program, as you can see in one of your tests.
Moreover if you use bozo here you will be able to further simplify this function

HenadziStantchik · 2019-11-28T12:56:55Z

+        logging.basicConfig(format='%(asctime)s %(message)s', datefmt='%I:%M:%S %p',
+                            stream=sys.stdout, level=logging.DEBUG)
+    else:
+        logging.basicConfig(format='%(asctime)s %(message)s', datefmt='%I:%M:%S %p',
+                            filename="sample.log", level=logging.DEBUG)


These two logging line are almost the same, the only difference is in 1 keyword argument. It is better to use separate functions in these situations, but it is not mandatory here.

HenadziStantchik · 2019-11-28T12:58:40Z

+        if args.version:
+            result = f'RSS reader version {open("VERSION.txt").readline()}'


It would be better if you put it instead of pass before

HenadziStantchik · 2019-11-28T13:07:46Z

+include rss_reader\TimesNewRoman.ttf
+include rss_reader\VERSION.txt


Because Linux and Windows have different filesystem separators, your setup installation will not work on Linux.

HenadziStantchik · 2019-11-28T13:33:55Z

Here is feedback on your app:
Iteration 1:
Everything works fine.
Iteration 2:
Everything seems working (but Windows-only, check my comment on your MANIFEST file)
Iteration 3:
Everything works fine, good job on implementing search by rss source.
Iteration 4:
Everything works fine
Iteration 5:
After using --colorize once colour of text in command line changes permanently (Linux) it would be nice to have it changed back. Aside from that everything is fine.

One test does not work: test_rssfile_to_dict, check my comment on your work_with_feedparser.py file. But despite one test not working, coverage rate is still very high.

The code is clean, easy to read, well structured and mostly corresponds to the PEP8 style recommendations. Documentation is a bit lacking, it would be nice if you could put smaller examples here and focus more on describing your project, for example how to install it and etc.

Commit messages mostly correspond to the recommended commit guidelines, but lacking in information.

protosevichVlad added 2 commits November 3, 2019 16:00

Iteration 3

36f2db1

Add README and bug fixes

0505252

protosevichVlad changed the title ~~Protosevich_Vlad_protosevic2001@gmail.com~~ Vlad_Protosevich_protosevich2001@gmail.com Nov 5, 2019

HenadziStantchik added the please review I want mentors to review my PR ASAP label Nov 5, 2019

HenadziStantchik reviewed Nov 5, 2019

View reviewed changes

Comment thread final_task/README.md

HenadziStantchik reviewed Nov 5, 2019

View reviewed changes

Comment thread final_task/rss_reader/rss_reader.py Outdated

HenadziStantchik reviewed Nov 5, 2019

View reviewed changes

Comment thread final_task/rss_reader/rss_reader.py Outdated

HenadziStantchik reviewed Nov 5, 2019

View reviewed changes

Comment thread final_task/rss_reader/rss_reader.py Outdated

HenadziStantchik added reviewed Review was performed and removed please review I want mentors to review my PR ASAP reviewed Review was performed labels Nov 5, 2019

splitting into files and fixing bugs

8cfbcc9

protosevichVlad changed the title ~~Vlad_Protosevich_protosevich2001@gmail.com~~ Vlad_Protosevich_protosevich2001@gmail.com - PLEASE REVIEW Nov 7, 2019

HenadziStantchik reviewed Nov 8, 2019

View reviewed changes

Comment thread final_task/rss_reader/rss_reader.py Outdated

HenadziStantchik reviewed Nov 8, 2019

View reviewed changes

Comment thread final_task/rss_reader/__init__.py Outdated

HenadziStantchik reviewed Nov 8, 2019

View reviewed changes

Comment thread final_task/rss_reader/rss_reader.py Outdated

HenadziStantchik reviewed Nov 8, 2019

View reviewed changes

Comment thread final_task/rss_reader/work_with_text.py Outdated

HenadziStantchik reviewed Nov 8, 2019

View reviewed changes

Comment thread final_task/rss_reader/work_with_text.py

HenadziStantchik changed the title ~~Vlad_Protosevich_protosevich2001@gmail.com - PLEASE REVIEW~~ Vlad_Protosevich_protosevich2001@gmail.com Nov 8, 2019

Add tests and bug fixes

6bf9b5d

Fix ModuleNotFoundError

5cf536a

protosevichVlad changed the title ~~Vlad_Protosevich_protosevich2001@gmail.com~~ Vlad_Protosevich_protosevich2001@gmail.com - PLEASE REVIEW Nov 8, 2019

HenadziStantchik changed the title ~~Vlad_Protosevich_protosevich2001@gmail.com - PLEASE REVIEW~~ Vlad_Protosevich_protosevich2001@gmail.com Nov 8, 2019

4 Iteration

f53c829

HenadziStantchik reviewed Nov 18, 2019

View reviewed changes

Comment thread final_task/rss_reader/MyException.py Outdated

HenadziStantchik reviewed Nov 18, 2019

View reviewed changes

Comment thread final_task/rss_reader/rss_reader.py Outdated

HenadziStantchik reviewed Nov 18, 2019

View reviewed changes

Comment thread final_task/rss_reader/work_with_file.py Outdated

HenadziStantchik reviewed Nov 18, 2019

View reviewed changes

Comment thread final_task/rss_reader/work_with_file.py Outdated

HenadziStantchik reviewed Nov 18, 2019

View reviewed changes

Comment thread final_task/rss_reader/work_with_html.py

HenadziStantchik changed the title ~~Vlad_Protosevich_protosevich2001@gmail.com - PLEASE REVIEW~~ Vlad_Protosevich_protosevich2001@gmail.com Nov 18, 2019

Fixed bugs: empty string in links, edit name exception

9ea80e5

protosevichVlad changed the title ~~Vlad_Protosevich_protosevich2001@gmail.com~~ Vlad_Protosevich_protosevich2001@gmail.com - PLEASE REVIEW Nov 19, 2019

HenadziStantchik changed the title ~~Vlad_Protosevich_protosevich2001@gmail.com - PLEASE REVIEW~~ Vlad_Protosevich_protosevich2001@gmail.com Nov 19, 2019

Fixed bugs

2ec5eac

protosevichVlad changed the title ~~Vlad_Protosevich_protosevich2001@gmail.com~~ Vlad_Protosevich_protosevich2001@gmail.com - PLEASE REVIEW Nov 19, 2019

HenadziStantchik changed the title ~~Vlad_Protosevich_protosevich2001@gmail.com - PLEASE REVIEW~~ Vlad_Protosevich_protosevich2001@gmail.com Nov 20, 2019

Add tests

a877463

protosevichVlad changed the title ~~Vlad_Protosevich_protosevich2001@gmail.com~~ Vlad_Protosevich_protosevich2001@gmail.com - PLEASE REVIEW Nov 23, 2019

dzhigailo reviewed Nov 23, 2019

View reviewed changes

dzhigailo changed the title ~~Vlad_Protosevich_protosevich2001@gmail.com - PLEASE REVIEW~~ Vlad_Protosevich_protosevich2001@gmail.com Nov 23, 2019

Fix bugs

a92dded

dzhigailo added the CODE FREEZE label Nov 25, 2019

HenadziStantchik reviewed Nov 28, 2019

View reviewed changes

		if args.version:
		result = f'RSS reader version {open("VERSION.txt").readline()}'

		include rss_reader\TimesNewRoman.ttf
		include rss_reader\VERSION.txt No newline at end of file

Conversation

protosevichVlad commented Nov 5, 2019

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

HenadziStantchik commented Nov 5, 2019 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

protosevichVlad commented Nov 7, 2019

Uh oh!

HenadziStantchik commented Nov 8, 2019

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

HenadziStantchik commented Nov 8, 2019

Uh oh!

protosevichVlad commented Nov 8, 2019

Uh oh!

HenadziStantchik commented Nov 8, 2019

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

HenadziStantchik commented Nov 18, 2019

Uh oh!

protosevichVlad commented Nov 18, 2019

Uh oh!

protosevichVlad commented Nov 18, 2019

Uh oh!

HenadziStantchik commented Nov 19, 2019

Uh oh!

protosevichVlad commented Nov 19, 2019

Uh oh!

HenadziStantchik commented Nov 20, 2019

Uh oh!

protosevichVlad commented Nov 23, 2019

Uh oh!

dzhigailo Nov 23, 2019

Choose a reason for hiding this comment

Uh oh!

HenadziStantchik Nov 28, 2019

Choose a reason for hiding this comment

Uh oh!

HenadziStantchik Nov 28, 2019

Choose a reason for hiding this comment

Uh oh!

HenadziStantchik Nov 28, 2019

Choose a reason for hiding this comment

Uh oh!

HenadziStantchik Nov 28, 2019

Choose a reason for hiding this comment

Uh oh!

HenadziStantchik commented Nov 28, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

HenadziStantchik commented Nov 5, 2019 •

edited

Loading