Denis_Marfonov_marfonovdenis@gmail.com by scarydzik · Pull Request #32 · epam-python-courses-7-bsu/FinalTaskRssParser

scarydzik · 2019-11-25T20:39:00Z

Iteration 4 without unittests and docstrings

Created CLI using argparse and handle all arguments from the first iteration.

Created Feed class for parsed RSS. For now, it only stores necessary fields from object returned by feedparser library.

Created module with function for parsing html, which can be in "description" field of RSS item.

Added methods for generating json and plain text with links list to Feed class

Added logging with ERROR level to display handled custom exceptions; and INFO level to display verbose status messages if corresponding argument is provided.

This is done in order to add main() function to 'console scripts' in setup.py

Added __init__ to make correct python package. Created __main__ to allow starting package as program Added content to setup.py.

If db is not exist or invalid, it is (re-)created. Feed class become context manager, it no longer contains heavy methods in init. Also, it helps to close db connector properly. Finally, slightly change plain text conversion.

HenadziStantchik · 2019-12-03T12:04:05Z

@@ -0,0 +1,3 @@
+from .rss_reader import main


It is better to use simple import in this case

HenadziStantchik · 2019-12-03T12:05:52Z

+    return html
+
+
+def _render_item(item):


Here and below: Usually underscore should be used either inside classes for method names or for variables that override nuilt-in names. You do not actually need to use it every time.

HenadziStantchik · 2019-12-03T12:06:06Z

+    html.append(title)
+    html.append("</h1>")
+
+    for i in items:


Here and below, it is better to avoid one letter variable names. No matter how simple statement is.

HenadziStantchik · 2019-12-03T12:07:12Z

+
+
+def _create_html(book_id, bookpath, text):
+    # text = html.escape(text)


It is better to avoid commented-out code in pushed commits.

HenadziStantchik · 2019-12-03T12:08:28Z

+        file = open(bookpath, "w", encoding="utf-8")
+    try:
+        file.write(text)
+        file.close()


It is better to use context manager for this instead of manually opening-closing file descriptor.

HenadziStantchik · 2019-12-03T12:09:07Z

+        file.write(text)
+        file.close()
+    except (AttributeError, OSError):
+        pass


Usually if you leave pass in the except block it is better to leave a comment why.

HenadziStantchik · 2019-12-03T12:11:44Z

+class FeedError(Exception):
+    pass
+
+
+class URLFormatError(FeedError):
+    pass
+
+
+class FeedNotFoundError(FeedError):
+    pass
+
+
+class IncorrectRSSError(FeedError):
+    pass
+
+
+class LocalCacheError(FeedError):
+    pass


It is better to move your custom exceptions to the separate module

HenadziStantchik · 2019-12-03T12:14:13Z

+    def __len__(self) -> int:
+        return len(self.items_dict)
+
+    def __contains__(self, __x: object) -> bool:
+        return __x in self.items_dict
+
+    def __iter__(self):
+        return self.items_dict.__iter__()
+
+    def __init__(self, initial=None):
+        self.items_dict = dict()
+        self._next_index_ = 1
+
+        if initial is not None:


It is better to be consistent with typehint usage: either typehint everywhere or nowhere.

HenadziStantchik · 2019-12-03T12:29:15Z

+import argparse
+import logging
+import time
+import os.path


Unused import here

HenadziStantchik · 2019-12-03T12:30:24Z

@@ -0,0 +1,110 @@
+import time
+import os.path
+import html


Unused import here

HenadziStantchik · 2019-12-03T12:43:31Z

Here is feedback on your application:
Because you used from . in your imports, your app does not work if user will try to launch it using python rss_reader.py <rss url> ... (or something similar).
However, after package installation, your app still does not work (see Iteration 2 pic)
So in order to test your app I needed to change most of your imports.
Iteration 1:
After changing imports, does not work:

Iteration 2:
Installs nice, but does not work:

Iteration 3:
Unable to test.
Iteration 4:
Unable to test

Commits are informative and correspond to the recommended commit message style.
There are no tests.
Application is not in working condition, tested both on a clean machine and inside isolated docker container.
Overall code is readable, but has some issues specified in comments. It is however absolutely corresponds to the PEP8 guidelines.

scarydzik · 2019-12-03T16:02:47Z

Because you used from . in your imports, your app does not work if user will try to launch it using python rss_reader.py ... (or something similar).

Program should be run with -m argument, as specified in the README:

$ python -m rss_reader ...

and from folder final_tast/rss_reader or from file __main__.py.
When I used absolute imports (import foo instead of from . import foo), console script from setup.py (rss-reader) didn't work.

As for the installation from PyPi, iteration 3 has version 0.3.post1 (because i accidently uploaded incompleted project as 0.3); iteration 4 was not added to TestPyPi, because I finished it just before deadline and forgot about it. Now I've added 4th iteration to TestPyPi.
Also it mandatory to install dependencies manualy. 4th iteration required ebooklib, which is not mentioned in README (cause I forgot), but is mentioned in requirements.txt and setup.py.

If it's possible, can you please re-test the application?

HenadziStantchik · 2019-12-04T12:21:56Z

Ok, my bad not reading your README.md attentively.

Your app still does not completely work though:

After a bit of investigating, I figured that the problem is that you are trying to connect/create to the DB file that is situated (in my case) in final_task/rss_reader/data/ folder. The problem is that I do not have this folder at all, so it fails to open DB connection. However after creating data folder, everything works fine.

The same problem with your installed package

So here is feedback on your app:
Iteration 1:
--json contains html code, aside from that, everything is fine.
Iteration 2:
Installs fine, all iterations work the same as using python -m ....
Iteration 3:
Everything works fine.
Iteration 4:
Html conversion works fine.
Epub conversion works fine, but contain no images, only placeholders for them:

scarydzik added 30 commits November 23, 2019 20:17

Create basic CLI with arguments parsing

3f9190c

Created CLI using argparse and handle all arguments from the first iteration.

Create class for RSS feed with basic parsing functionality

6cbe140

Created Feed class for parsed RSS. For now, it only stores necessary fields from object returned by feedparser library.

Create module for html parsing

7adeb98

Created module with function for parsing html, which can be in "description" field of RSS item.

Add html parsing to Feed class

8df0ad3

Add methods for generating plain text and json from RSS

9ab49d1

Added methods for generating json and plain text with links list to Feed class

Remove unnecessary parameter from Feed constructor

498b4f8

Fix relative module import error

67ed39c

Add RSS parsing and printing and handle custom exceptions

5acb43d

Add logging and implement --verbose argument

b93e90b

Added logging with ERROR level to display handled custom exceptions; and INFO level to display verbose status messages if corresponding argument is provided.

Rename internal methods and remove incorrect docstring

fcbf10f

Update README

914c17b

Add LICENSE and update README

2bc7410

Move code to separate function

24c0cc6

This is done in order to add main() function to 'console scripts' in setup.py

Make correct package from code folder and fill setup.py

9f214b5

Added __init__ to make correct python package. Created __main__ to allow starting package as program Added content to setup.py.

Add dependencies list and reformat file

cb62489

Fix typo

0d63bc4

Change imports to relative

7c50244

Update README

9ec3ade

Make CLI always show correct program name

53e0a60

Fill requirements.txt

26cf900

Bump version number to 0.2

ca5ae76

Add --date argument to CLI

80e321b

Implement local caching using sqlite database

7944473

Add check for db, turn main class into context manager

aa24f8b

If db is not exist or invalid, it is (re-)created. Feed class become context manager, it no longer contains heavy methods in init. Also, it helps to close db connector properly. Finally, slightly change plain text conversion.

Add DB exceptions handle(re-raising) to main class

9628022

Update README and slightly change json output

8ae5afd

Bumps version number

ef7f12a

Make post-release to correct mistake

7f63ba8

Add epub and html creating

4eda8c6

Update README

665e65b

Bump version number and add new requirement

8553cf4

dzhigailo added the CODE FREEZE label Nov 25, 2019

HenadziStantchik reviewed Dec 3, 2019

View reviewed changes



		def _create_html(book_id, bookpath, text):
		# text = html.escape(text)

Conversation

scarydzik commented Nov 25, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

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

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

HenadziStantchik commented Dec 3, 2019

Uh oh!

scarydzik commented Dec 3, 2019

Uh oh!

HenadziStantchik commented Dec 4, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

HenadziStantchik Dec 3, 2019 •

edited

Loading