Anastasiya_Merkushova_st.merkush@gmail.com - PLS REVIEW by clonder · Pull Request #31 · epam-python-courses-7-bsu/FinalTaskRssParser

clonder · 2019-11-24T21:39:13Z

1-5 done (without test), fix some problems with commits.

dzhigailo · 2019-11-25T08:48:38Z

+        return cls._instances[cls]
+
+
+class DatabaseEmulation(metaclass=Singleton):


What is the reason to implement this class as Singleton? You create it only once anyway.

It’s a good practice to make such objects as singleton. It doesn’t matter that in this case the object of this class is used once, it is much more important that if it is used a lot where, it was the same object.

dzhigailo · 2019-11-25T08:52:49Z

+        self.write_data_stream = open(self.path_to_data, "ab")
+        self.read_data_stream = open(self.path_to_data, "rb")
+
+    def __exit__(self, *args):


This magic method doesn't make any sense without __enter__(self) magic method since both of those method are required for context manager protocol. Moreover, you never call it and opened files will not be closed.
Magic method exit always have 3 positional arguments. There is no reason to use *args.

I didn’t know about this, thanks!
Fixed.

dzhigailo · 2019-11-25T08:55:44Z

+        self.dates_file.write("{} {}\n".format(date, idx))
+
+
+def get_txt_date(data) -> None:


Function called get_txt_date, but it doesn't return anything.

Sorry, missed this! Already fixed!

dzhigailo · 2019-11-25T08:57:09Z

@@ -0,0 +1,93 @@
+import pickle


Module name doesn't not follow PEP standard. It should be in lower case.

Fixed, thanks!

dzhigailo · 2019-11-25T08:58:18Z

+
+def get_txt_date(data) -> None:
+    date_id = {}
+    with open("dates.txt", "r", encoding="utf-8") as f:


"r", encoding="utf-8" these are default parameters for open() function.

Same for code below.

Fixed this, Thanks!

dzhigailo · 2019-11-25T09:40:52Z

+                if element.endswith(".png") or element.endswith(".jpg"):
+                    image = r.get(element)
+                    try:
+                        img_path = str(len(element)) + str(random.randint(1, 128)) + str(img_url[-4:])


Here and below: you can't use random in path generation.

dzhigailo · 2019-11-25T09:43:03Z

+        ' '.join(item_xml['content']),
+        1
+    )
+    prepared_news = OrderedDict()


In python version 3.7+ dictionary is ordered by default.

I know this, but if someone accidentally uses python3.6 for example, it will not be good if he has undefined behavior.

dzhigailo · 2019-11-25T09:46:51Z

+
+    log.info("Start script")
+
+    database = DatabaseEmulation.DatabaseEmulation(


This class should support context manager protocol and use it.

dzhigailo · 2019-11-25T09:51:31Z

First two commits should be squashed.
Tests are required by task definition.

…nd verification(PEP) is done using flake8

HenadziStantchik · 2019-12-03T16:14:12Z

+arguments==76
+beautifulsoup4==4.8.1
+bs4==0.0.1
+certifi==2019.9.11
+cffi==1.13.2
+chardet==3.0.4
+colorama==0.4.1
+colorize==1.1.0
+cycler==0.10.0
+dominate==2.4.0
+entrypoints==0.3
+feedparser==5.2.1
+flake8==3.7.9
+fpdf==1.7.2
+idna==2.8
+kiwisolver==1.1.0
+lxml==4.4.1
+mccabe==0.6.1
+numpy==1.17.4
+pycodestyle==2.5.0
+pycparser==2.19
+pyflakes==2.1.1
+pyparsing==2.4.5
+python-dateutil==2.8.1
+requests==2.22.0
+six==1.13.0
+soupsieve==1.9.5
+urllib3==1.25.7


There are too many redundant libs here. Keep in mind that you should specify only libs that are directly required by your application.

HenadziStantchik · 2019-12-03T17:13:15Z

+<<<<<<< HEAD
+=======
+
+>>>>>>> a9c6fd52b1a59d83145cc3e869cfe9f7a0a6c2fe


You have unresolved conflicts here.

HenadziStantchik · 2019-12-03T17:14:15Z

+    parser = argparse.ArgumentParser(
+        description='Pure Python command-line RSS reader.',
+        prog='rss-reader')


Here and below: it is better to make this into one line, it will be still pretty readable.
Also it is better to use f"" instead of .format().

HenadziStantchik · 2019-12-03T17:14:54Z

@@ -0,0 +1,146 @@
+import logging as log
+import requests as r


It is better to avoid one-letter variable names.

HenadziStantchik · 2019-12-03T17:15:57Z

+                            img_path = str(len(element)) + \
+                                       str(img_url[-4:])


It is better to use os.path for operations with filesystem paths because Windows OS and Linux OS have different separators

HenadziStantchik · 2019-12-03T17:17:20Z

+        log.info("Successful converting into html")
+
+
+def create_pdf(items: list) -> None:


This function is too large. It is better to split it.

HenadziStantchik · 2019-12-03T17:21:06Z

+class Singleton(type):
+    _instances = {}
+
+    def __call__(cls, *args, **kwargs):
+        if cls not in cls._instances:
+            cls._instances[cls] = super(Singleton, cls).__call__(
+                *args,
+                **kwargs
+            )
+        return cls._instances[cls]


It is better to not use such thing as singleton in this app. It is not needed at all.

HenadziStantchik · 2019-12-03T17:22:20Z

+    def __del__(self):
+        self.dates_file.close()
+        self.write_data_stream.close()
+        self.read_data_stream.close()


It is better to use context manager's __enter__ and __exit__ for this purpose, not __del__

HenadziStantchik · 2019-12-03T17:22:43Z

+        print("There isn't any news from this day")
+
+
+def cash_news() -> None:


A typo here: cache

HenadziStantchik · 2019-12-03T17:23:24Z

+
+
+def filter_title(title: str) -> str:
+    new_title = title.replace("&#39;", '')


It is better to have a bit more universal solution, as there might be other troublesome symbols.

HenadziStantchik · 2019-12-03T17:23:45Z

+    month = {
+        'Dec': '12', 'Jan': '01', 'Feb': '02',
+        'Mar': '03', 'Apr': '04', 'May': '05',
+        'Jun': '06', 'Jul': '07', 'Aug': '08',
+        'Sep': '09', 'Oct': '10', 'Nov': '11'
+    }
+    new_date = date


It is better to use datetime lib for this

HenadziStantchik · 2019-12-03T17:35:34Z

Here is feedback on your application:
It does not work right away

It does not work if installed using setuptools either (using python setup.py install):

Your app was tested both on clean machine and inside docker container.

The code is mostly readable, corresponds to the PEP8 guidelines.
Tests are almost nonexistent and coverage percentage is barely high enough.
Commits mostly are not informative enough and do not correspond to the recommended commit message style.

clonder · 2019-12-04T15:00:17Z

Hi
1)I found a problem in the fact that I have a file dates in the folder _database in which for some reason there were strange lines with an error. (and it booted). If you delete them, my whole program works.
If there is such an opportunity - please look again. (I try it on two other machines)
2) for other screen I think you should try "python3 rss_reader.py -h" with _

clonder added 3 commits November 25, 2019 00:30

Made 5 iterations without tests. All comments taken into account.

fd57e18

Made 5 iterations without tests. All comments taken into account.

ba26882

fix gitignore

e1bb3b8

dzhigailo reviewed Nov 25, 2019

View reviewed changes

dzhigailo changed the title ~~Anastasiya_Merkushova_st.merkush@gmail.com - PLS REVIEW~~ Anastasiya_Merkushova_st.merkush@gmail.com Nov 25, 2019

All comments were corrected and taken into account. Tests are added a…

3e5d173

…nd verification(PEP) is done using flake8

clonder changed the title ~~Anastasiya_Merkushova_st.merkush@gmail.com~~ Anastasiya_Merkushova_st.merkush@gmail.com - PLS REVIEW Nov 25, 2019

clonder and others added 5 commits November 25, 2019 17:45

Update dates.txt

95179dd

Update dates.txt

3cc1a3e

Update dates.txt

a9c6fd5

Fix critical bug with -pdf

d43b269

Fix critical bug with -pdf

3608fed

dzhigailo added the CODE FREEZE label Nov 25, 2019

HenadziStantchik reviewed Dec 3, 2019

View reviewed changes

Update dates.txt

85341d6

		return cls._instances[cls]


		class DatabaseEmulation(metaclass=Singleton):

		self.dates_file.write("{} {}\n".format(date, idx))


		def get_txt_date(data) -> None:


		log.info("Start script")

		database = DatabaseEmulation.DatabaseEmulation(

		log.info("Successful converting into html")


		def create_pdf(items: list) -> None:

		print("There isn't any news from this day")


		def cash_news() -> None:



		def filter_title(title: str) -> str:
		new_title = title.replace("'", '')

Conversation

clonder commented Nov 24, 2019

Uh oh!

Choose a reason for hiding this comment

Uh oh!

clonder Nov 25, 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!

clonder Nov 25, 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!

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!

dzhigailo commented Nov 25, 2019

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!

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!

clonder commented Dec 4, 2019

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

clonder Nov 25, 2019 •

edited

Loading

clonder Nov 25, 2019 •

edited

Loading