Dvorakouski Kiryl #37

KirylDv · 2019-11-10T18:12:31Z

I'm not sure should I write Dvarakouski Kiryl or Dvorakouski Kiryl

AlexeiBuzuma · 2019-11-27T19:27:42Z

project/html_parser.py

+        self.links = []
+        self.text = ""
+
+    def handle_starttag(self, tag, attrs):


Описываю ситуацию. Я открываю пулл реквест, первым делом я вижу вот эту функцию. Я не смотрел остальной код, на данный момент еще не разбирался. Пытаюсь разобраться с этой функцией.
Так вот, я совсем не могу понять, что эта функция делает, для чего она предназначена и т.д., пока я не просмотрю весь код или даже не начну гонять этот код с дебаггером.
Хотя бы подробный док стринг написать, что оно делает, пояснить, какие аргументы приходят (это как минимум)

AlexeiBuzuma · 2019-11-27T19:28:08Z

project/html_parser.py

+from html.parser import HTMLParser
+
+
+class _html_parser(HTMLParser):


Очень странное название класса, должен быть CamelCase

AlexeiBuzuma · 2019-11-27T19:28:58Z

project/reader.py

+        print(string, sep=sep, end=end, flush=flush)
+
+
+def progress(elems, done, length=20):


название функций и методов должны показывать действие (в самом простом случае содержать глагол). названия output и progress очень плохо подходят как имена функций

AlexeiBuzuma · 2019-11-27T19:35:42Z

project/reader.py

+                output("Error: Can't connect to web-site, please check URL")
+            else:
+                output("Unknown error")
+            sys.exit()


Лучше будет использовать уже встроенный функционал для обработки исключений и прописать несколько except блоков, где будет обрабатываться каждое из этих исключений

AlexeiBuzuma · 2019-11-27T19:40:16Z

project/reader.py

+    show_news() - output news to stdout
+    """
+
+    def __init__(self, args):


В данном случае интерфейс класса очень сильно завязан на объект args, который содержит аргументы командной строки.
Если завтра придется избавиться от argparse, то придется и этот класс переписывать.
я бы посоветовал просто принимать 4 этих значения как аргументы init вместо объекта args

AlexeiBuzuma · 2019-11-27T19:41:54Z

project/reader.py

+            output(f"Title: {news[0]}")
+            output(f"Date: {news[1]}")
+            output(f"Link: {news[2]}", end="\n\n")
+            output(news[3], end="\n\n")


очень много магических переменных, которые что-то значат и скорее всего что-то важное, но я не могу понять, какой смысл в них скрывается :)
Как минимум имеет смысл эти индексы вынестси в отдельные переменные, а лучше подумать, как можно красивее работать с этими объектами news

AlexeiBuzuma · 2019-11-27T19:43:53Z

project/reader.py

+            if separ:
+                json += ','
+            separ = True
+            json += '{\n      "title": "' + news[0] + '",'


обычно советуют избегать использовать в качестве имени переменных/функций и т.д. названия модулей (в пайтоне есть модуль json)
лучше переименовать эту переменную

AlexeiBuzuma · 2019-11-27T19:44:42Z

project/reader.py

+
+def output(string, sep=' ', end='\n', flush=False, verbose=True):
+    """Output function for singe string but convert &#39; to '"""
+    if verbose:


для этого лучше использовать функциональность модуля logging

AlexeiBuzuma · 2019-12-07T20:47:35Z

--json перестал работать

# rss_reader https://news.tut.by/rss/index.rss --json 
Traceback (most recent call last):
  File "/usr/local/bin/rss_reader", line 8, in <module>
    sys.exit(main())
  File "/usr/local/lib/python3.8/site-packages/project/rss_reader.py", line 26, in main
    rss.show_json()
  File "/usr/local/lib/python3.8/site-packages/project/reader.py", line 136, in show_json
    json_text = Converter.to_json(feed, column, self.__verbose, color=self.__colorize)
TypeError: to_json() got an unexpected keyword argument 'color'

AlexeiBuzuma · 2019-12-07T20:48:45Z

Сгенерировать HTML не получается

# rss_reader https://news.tut.by/rss/index.rss --to_html
Traceback (most recent call last):
  File "/usr/local/bin/rss_reader", line 8, in <module>
    sys.exit(main())
  File "/usr/local/lib/python3.8/site-packages/project/rss_reader.py", line 32, in main
    rss.save_html()
  File "/usr/local/lib/python3.8/site-packages/project/reader.py", line 154, in save_html
    Converter().to_html(feed, column, verbose=self.__verbose, color=self.__colorize)
  File "/usr/local/lib/python3.8/site-packages/project/converter.py", line 203, in to_html
    img_path = _download_image(link, verbose)
TypeError: _download_image() missing 1 required positional argument: 'color'
root@e646f5762ed4:/data/PythonHomework# rss_reader https://news.tut.by/rss/index.rss --to_html /data/my_news
usage: rss_reader [-h] [--version] [--json] [--verbose] [--to_fb2] [--to_html] [--colorize] [--path PATH] [--limit LIMIT] [--date DATE] source
rss_reader: error: unrecognized arguments: /data/my_news

AlexeiBuzuma · 2019-12-07T20:49:17Z

Такая же ситуация с fb2

AlexeiBuzuma · 2019-12-07T20:50:41Z

есть небольшие стилистические ошибки

root@e646f5762ed4:/data/PythonHomework# pycodestyle --max-line-length=120 .
./project/SQL_cache.py:19:66: W291 trailing whitespace
./project/SQL_cache.py:20:50: W291 trailing whitespace
./project/SQL_cache.py:39:37: W291 trailing whitespace
./project/SQL_cache.py:49:40: W291 trailing whitespace
./project/SQL_cache.py:51:41: W291 trailing whitespace
./project/converter.py:70:50: W291 trailing whitespace
./project/converter.py:81:82: W291 trailing whitespace
./project/converter.py:95:21: W291 trailing whitespace
./project/reader.py:57:9: E731 do not assign a lambda expression, use a def
./project/reader.py:66:83: W291 trailing whitespace
./project/reader.py:143:76: W291 trailing whitespace
./project/rss_reader.py:14:37: E231 missing whitespace after ','

AlexeiBuzuma · 2019-12-11T10:28:58Z

# rss_reader  https://news.tut.by/rss/world.rss --limit 1
Traceback (most recent call last):
  File "/usr/local/bin/rss_reader", line 8, in <module>
    sys.exit(main())
  File "/usr/local/lib/python3.8/site-packages/project/rss_reader.py", line 23, in main
    if ' ' in args.path:
TypeError: argument of type 'NoneType' is not iterable

AlexeiBuzuma · 2019-12-11T10:49:22Z

ошибка при попытке сгенерировать pdf

# rss_reader  https://news.tut.by/rss/world.rss --to_html --path /data/news
Traceback (most recent call last):
  File "/usr/local/bin/rss_reader", line 8, in <module>
    sys.exit(main())
  File "/usr/local/lib/python3.8/site-packages/project/rss_reader.py", line 43, in main
    rss.save_html()
  File "/usr/local/lib/python3.8/site-packages/project/reader.py", line 152, in save_html
    Converter().to_html(feed, column, self.__sv_path, verbose=self.__verbose, color=self.__colorize)
  File "/usr/local/lib/python3.8/site-packages/project/converter.py", line 203, in to_html
    img_path = _download_image(link, verbose, sv_path, color)
  File "/usr/local/lib/python3.8/site-packages/project/converter.py", line 14, in _download_image
    local_name, headers = urllib.request.urlretrieve(
  File "/usr/local/lib/python3.8/urllib/request.py", line 257, in urlretrieve
    tfp = open(filename, 'wb')
FileNotFoundError: [Errno 2] No such file or directory: '/data/news/vulkan_novaya_zelandiya_09122019_3.jpg'

AlexeiBuzuma · 2020-01-09T14:36:18Z

project/SQL_cache.py

+    """Class working with SQLite3 database"""
+
+    def __init__(self):
+        super(Database, self).__init__()


В данном случае эта строка не нужна.

AlexeiBuzuma · 2020-01-09T14:37:52Z

project/SQL_cache.py

+
+    def __init__(self):
+        super(Database, self).__init__()
+        if not exists("cache.db"):


Советую название файла с кэшем вынести в какую-нибудь переменную, к которой можно обратиться из всех мест, где оно необходимо. Например сделать ее отрибутом класса (либо сделать как аргумент функции init, в таком случае эта функциональность будет более кастомизируема.

AlexeiBuzuma · 2020-01-09T14:39:02Z

project/SQL_cache.py

+        try:
+            self._open()
+            self.cursor.execute(f"""
+                SELECT name from feed WHERE source = '{url}'


Тут есть опасность SQL инъекций (https://ru.wikipedia.org/wiki/%D0%92%D0%BD%D0%B5%D0%B4%D1%80%D0%B5%D0%BD%D0%B8%D0%B5_SQL-%D0%BA%D0%BE%D0%B4%D0%B0)

AlexeiBuzuma · 2020-01-09T14:42:05Z

project/reader.py

+        if self.__verbose:
+            write_progressbar(len(data), counter)
+        for news in data:
+            column += [{"title": news[2],


Чтобы не было магических чисел в коде, можно использовать объектную модель хранения данных. В данном случае возможно подойдет использование namedtuple

AlexeiBuzuma · 2020-01-09T14:43:03Z

project/reader.py

+    def __cache_data(self, column, feed):
+        """Take parsed data and write it to database"""
+        stdout_write("Writing data to database...", verbose=self.__verbose, color="blue", colorize=self.__colorize)
+        date = lambda pubDate: dateutil.parser.parse(pubDate).strftime("%Y%m%d")


выглядит достаточно сложным решением. Не думаю, что в данном случае необходимо использовать лямбду

AlexeiBuzuma · 2020-01-09T14:44:59Z

project/reader.py

+from .SQL_cache import Database
+
+
+class RSSReader():


Данный класс немного перегружен по функциональности.
Класс RSSReader должен заниматься чтением RSS. Однако в данном случае он также занимается сохранением в новостей в различные форматы и выводом новостей в консоль.
Данный класс стоит разделить на несколько классов

AlexeiBuzuma · 2020-01-09T14:46:07Z

project/rss_reader.py

@@ -0,0 +1,51 @@
+#!/usr/bin/env python3


Отсутствуют юнит-тесты

AlexeiBuzuma · 2020-01-09T14:59:13Z

project/converter.py

+            if separ:
+                json_text += ','
+            separ = True
+            json_text += '{\n      "title": "' + news['title'] + '",'


Есть модуль json, в котором есть функциональность по конвертированию объектов Python в JSON представление.

AlexeiBuzuma · 2020-01-09T15:00:21Z

project/converter.py

+            text_links = []
+            for link in news["links"]:
+                if "(image)" in link:
+                    image_links += [link[:-8]]


Magic number

AlexeiBuzuma · 2020-01-09T15:01:55Z

project/reader.py

+    """
+
+    def __init__(self, source, limit, verbose, date, sv_path, colorize):
+        super(RSSReader, self).__init__()


Класс RSSReader не наследуется от других классов (кроме неявного наследования от object)
Данный вызов super не нужен

KirylDv and others added 15 commits November 10, 2019 19:39

Create rss_reader.py

f265ddb

Delete rss_reader.py

df71920

Create rss_reader.py

bb229b9

Add: arguments for parser

aec5dd7

Create file reader.py

7d412ef

Add: parser

416704e

Add: console output for news

e46c7cc

Add: --verbose

48baada

Add: call --json function

5a75dfb

Add: --json

4617b7d

Add shebang & correct codestyle

751a4d0

Fix: images without alt text

3dcb76a

1st Iteration

cce26c1

Add: setup.py

4ddda5c

2nd Iteration. Make changes for setup

92c78bf

AlexeiBuzuma added the [Deadline] Iteration 1-2 This is a marker for first and second iterations deadline. label Nov 17, 2019

Refactor convertion to json

c2abc7c

AlexeiBuzuma reviewed Nov 27, 2019

View reviewed changes

AlexeiBuzuma added the [1-2 iteration] Minimal requirements checked label Nov 27, 2019

AlexeiBuzuma reviewed Nov 27, 2019

View reviewed changes

KirylDv added 4 commits November 29, 2019 14:26

Add parts for cahce

f0aa494

Add: database exists checking, change news from list fo dict

ca5588c

Add: working with cache methods

5b0ba26

Add require packages

578e9e4

KirylDv added 7 commits December 1, 2019 16:09

Add: convertion to html

677b7bf

Add docstrings for html & fb2

b60845a

Add: verbose

01fb821

4th Iteration

fc84232

Add colors

68ff764

5th Iteration

c116d2e

5th Iteration. Strings of more than 120 characters removed

330002b

KirylDv changed the title ~~[WIP] Dvorakouski Kiryl~~ Dvorakouski Kiryl Dec 1, 2019

AlexeiBuzuma added the Hard Deadline label Dec 1, 2019

AlexeiBuzuma added the 3-4 iteration | not implemented label Dec 7, 2019

Fix errors added with colorize

2d5292c

AlexeiBuzuma removed Hard Deadline [Deadline] Iteration 1-2 This is a marker for first and second iterations deadline. labels Dec 12, 2019

AlexeiBuzuma reviewed Jan 9, 2020

View reviewed changes

project/rss_reader.py

@@ -0,0 +1,51 @@

#!/usr/bin/env python3

Copy link

Collaborator

AlexeiBuzuma Jan 9, 2020

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Отсутствуют юнит-тесты

AlexeiBuzuma reviewed Jan 9, 2020

View reviewed changes

AlexeiBuzuma added the Reviewed label Jan 9, 2020

		from html.parser import HTMLParser


		class _html_parser(HTMLParser):

		print(string, sep=sep, end=end, flush=flush)


		def progress(elems, done, length=20):

Dvorakouski Kiryl #37

Are you sure you want to change the base?

Dvorakouski Kiryl #37

Uh oh!

Conversation

KirylDv commented Nov 10, 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!

AlexeiBuzuma commented Dec 7, 2019

Uh oh!

AlexeiBuzuma commented Dec 7, 2019

Uh oh!

AlexeiBuzuma commented Dec 7, 2019

Uh oh!

AlexeiBuzuma commented Dec 7, 2019

Uh oh!

AlexeiBuzuma commented Dec 11, 2019

Uh oh!

AlexeiBuzuma commented Dec 11, 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!

Uh oh!