Skip to content

Pashkovskaya Polina #1

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 25 commits into
base: master
Choose a base branch
from

Conversation

pashkovskaya
Copy link

No description provided.

TeRRoRlsT added a commit to TeRRoRlsT/PythonHomework that referenced this pull request Nov 14, 2019
Added:
    * File htmlparser. Contain all parsing functionality of program.
    * Added docstrings in all files.
    * Added logging. To use write '--verbose' after filename.
    * Added converting to JSON. Also now available output all news articles in JSON format.

Change:
    * Functionality of class RSSReader. All parsing work removed into htmlparser module.

Fixed all files with PEP8
Comment on lines 18 to 30
data = feedparser.parse(url)
if data['bozo']:
raise ValueError("Wrong URL address or there is no access to the Internet")
self.feed = data['feed'].get('title', None)
entries = data['entries'] if count < 0 else data['entries'][:count]
self.items = []
for entry in entries:
title = unescape(entry.get('title', 'No title'))
date = entry.get('published', 'Unknown')
link = entry.get('link', 'No link')
html = entry.get('summary', None)
content = NewsContent.get_content_from_html(html)
self.items.append(NewsItem(title, date, link, content))
Copy link
Collaborator

Choose a reason for hiding this comment

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

Советую сделать тут небольшой рефакторинг.
Основная идея __init__ функции -- это базовая инициализация объекта.
В данном случае получается, что при создании объекта News, будут идти запросы на получение RSS, его парсинг и тд.
я бы посоветовал эту логику вынести в отдельные методы и где-нибудь явно их вызывать.

return len(self.items)


class NewsItem:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Это не критично, но как совет на будущее.
Выглядит так, что этот класс является обычным контейнером данных.
Для того, чтобы просто красиво хранить данные в какой-то структуре, можно использовать namedtuple из библиотеки collections или дата классы. (повторюсь, это дело вкуса)


logger.info('Program completed')

if args.verbose:
Copy link
Collaborator

Choose a reason for hiding this comment

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

Логгер в пайтоне можно настроить так, чтобы он писал логи одновременно в файл и в консоль.
Есть много способов, некоторые можно найти вот тут:
https://stackoverflow.com/questions/13733552/logger-configuration-to-log-to-file-and-print-to-stdout

result += str(item) + '\n\n'
return result

def json(self):
Copy link
Collaborator

Choose a reason for hiding this comment

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

Я бы переименовал название метода так, чтобы оно показывало действие
например to_json или convert_to_json

@AlexeiBuzuma AlexeiBuzuma added the [Deadline] Iteration 1-2 This is a marker for first and second iterations deadline. label Nov 17, 2019
with open("README.md", "r") as fh:
long_description = fh.read()

setuptools.setup(
Copy link
Collaborator

Choose a reason for hiding this comment

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

Не прописаны зависимости, из-за этого после установке в чистом докер контейнере падает вот такая ошибка:

# rss-reader --help
Traceback (most recent call last):
  File "/usr/local/bin/rss-reader", line 5, in <module>
    from rss_reader.rss_reader import main
  File "/usr/local/lib/python3.8/site-packages/rss_reader/rss_reader.py", line 12, in <module>
    from rss_reader.news import News
  File "/usr/local/lib/python3.8/site-packages/rss_reader/news.py", line 5, in <module>
    import feedparser
ModuleNotFoundError: No module named 'feedparser'

@@ -0,0 +1,72 @@
import os
Copy link
Collaborator

Choose a reason for hiding this comment

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

pycodestyle выдает несколько минимальных стилистических ошибок

# pycodestyle --max-line-length=120 .
./setup.py:14:17: E251 unexpected spaces around keyword / parameter equals
./setup.py:14:19: E251 unexpected spaces around keyword / parameter equals
./rss_reader/__init__.py:1:1: W391 blank line at end of file
./rss_reader/rss_reader.py:64:1: E302 expected 2 blank lines, found 1

@@ -0,0 +1,72 @@
import os
import json
Copy link
Collaborator

Choose a reason for hiding this comment

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

выводится первая версия продукта, хотя по идее как минимум вторая :)

# rss-reader --version
1.0

@pashkovskaya pashkovskaya changed the title [WIP] Pashkovskaya Polina Pashkovskaya Polina Dec 1, 2019
@AlexeiBuzuma AlexeiBuzuma removed Hard Deadline [Deadline] Iteration 1-2 This is a marker for first and second iterations deadline. labels Dec 12, 2019
@AlexeiBuzuma
Copy link
Collaborator

Код красивый, легко читается, в основном хорошо декомпозирован. Ниже приведены основные моменты по этой работе.

Хорошие моменты:

  1. Использование темплейтов для HTML конвертации
  2. Высокое покрытие тестами.
  3. Соответствие PEP8
  4. Хороший стиль git сообщений.
  5. Код чистый и хорошо читается, подробное документирование функций и классов

Что может быть улучшено:

  1. main функция очень большая, тут можно сделать небольшой рефакторинг
  2. действительно ли необходимо хранить там много файлов со шрифтами в репозитории?
  3. Хранение даты в JSON формате может быть более компактно организовано в виде строки вместо списка чисел

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants