Skip to content

Pashkevich Anton #32

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 11 commits into
base: master
Choose a base branch
from

Conversation

prague15031939
Copy link

@prague15031939 prague15031939 commented Nov 10, 2019

iterations 1-4 are completed

provided all details of cli interface required by technical task
import argparse
import requests

parser = argparse.ArgumentParser(description='Pure Python command-line RSS reader.', prog='rss_reader')
Copy link
Collaborator

Choose a reason for hiding this comment

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

В глобальной секции не должно быть логики. Советую перенести парсинг аргументов в отдельную функцию.

provided work logic for parsingrss feed and displaying news, --json --verbose and --limit optional arguments
cli utility 'rss_reader' was wrapped into distribution package
@AlexeiBuzuma AlexeiBuzuma added the [Deadline] Iteration 1-2 This is a marker for first and second iterations deadline. label Nov 17, 2019
provided caching news via database
updated README.md in order to illustrate database structure
with open("README.md", "r") as f:
long_description = f.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
после установки пакета через pip install . такой утилиты не появляется

# rss-reader
bash: rss-reader: command not found


return json.dumps({'news': news}, indent=2)

def main():
Copy link
Collaborator

Choose a reason for hiding this comment

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

Возможно стоит разбить эту функцию на несколько маленьких функций

"""
this function prints news in stdout
"""
if len(news) == 0:
Copy link
Collaborator

Choose a reason for hiding this comment

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

if not news:
    return None

try:
logger.info(f"checking date..")
if not cacher.is_valid_date(args.date):
raise ValueError
Copy link
Collaborator

Choose a reason for hiding this comment

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

есть смысл добавить текстовое описание к каждому исключению, которое тут вызывается

if args.limit:
if args.limit < 1:
if not args.verbose:
print("error: invalid limit value")
Copy link
Collaborator

Choose a reason for hiding this comment

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

А зачем для печати одного и того же текста использовать и функцию print и логгер?
По идее можно обойтись только логгером

provided converting to html and pdf, fixed issues with package distribution
provided downloading font neccessary for pdf format from the Internet
@prague15031939 prague15031939 changed the title [WIP] Anton Pashkevich Pashkevich Anton Dec 1, 2019
format_converter.to_html(news, args.html)
except (OSError, FileNotFoundError):
if len(logger.handlers) == 1:
logger.addHandler(logging.StreamHandler(sys.stdout))
Copy link
Collaborator

Choose a reason for hiding this comment

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

Все время говорит error: invalid directory

Copy link
Author

Choose a reason for hiding this comment

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

@AlexeiBuzuma я перепроверил, всё вроде как нормально

while text[i] != '\n':
i += 1
text = text[:i] + "<br>" + text[i + 1:]
i += 4
Copy link
Collaborator

Choose a reason for hiding this comment

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

Очень много магических чисел :)

break
f.write(block)

def to_pdf(news, filepath):
Copy link
Collaborator

Choose a reason for hiding this comment

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

Это достаточно большая функция. Есть смысл разделить ее на несколько маленьких

import requests
from fpdf import FPDF

def break_lines(text):
Copy link
Collaborator

Choose a reason for hiding this comment

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

Возможно я ошибаюсь, но может ли в данном случае подойти метод replace у строки?

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