Skip to content
Open
Show file tree
Hide file tree
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
160 changes: 0 additions & 160 deletions .gitignore

Choose a reason for hiding this comment

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

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

This file was deleted.

108 changes: 108 additions & 0 deletions main.py

Choose a reason for hiding this comment

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

💡 для проектов используй следующую структуру:

redditparser/__init__.py
redditparser/__main__.py
redditparser/client.py
redditparser/topfinder.py
tests
readme.md
...

Original file line number Diff line number Diff line change
@@ -0,0 +1,108 @@
import datetime

import httpx

Choose a reason for hiding this comment

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

👍 круто, что используешь httpx

from sensitive_data import client_id, client_secret, username, password

subreddit = 'learnpython'

Choose a reason for hiding this comment

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

💡 мы не пишим скрипты - мы пишем программы, приложения, сервисы - поэтому никаких глобальных переменных

💡 давай будем спрашивать о том сабреддите, который хотим спарсить у пользователя с помощью input()



def get_token(client_id: str, client_secret: str, username: str, password: str) -> str:

Choose a reason for hiding this comment

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

👍 хороший метод и аннотации верно проставлены
💡 знаешь, чего нам не хватает некоей изоляции методов которые отвечают за обращения в reddit от основной логики приложения, которая как раз считает топ авторов и комментаторов.

Поэтому, давай все функция, где используется httpx и запросы на адрес reddit.com вытащим сначала просто в отдельный модуль client.py, а затем сделаем класс RedditClient, в котором будут все эти методы. Более того, такие параметры как имя пользователя / секрет / пароль / ид клиента можно будет передавать в виде датакласса reddit_conf в __init__ этого класса

''' получить токен '''

Choose a reason for hiding this comment

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

💡 для docstring используй двойные кавычки

post_data = {"grant_type": "password", "username": username, "password": password}
headers = {"User-Agent": "User-Agent"}

Choose a reason for hiding this comment

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

💡 обрати внимание, реддит настоятельно требует, чтобы в имя user-agent было передано название агента по определенным правилам собранное - в нем должно фигурировать твое имя пользователя. Иначе реддит может и твои запросы банить и даже забанить аккаунт, как нарушение контракта апи .

response = httpx.post("https://www.reddit.com/api/v1/access_token",
auth=(client_id, client_secret), data=post_data, headers=headers)
return response.json()['access_token']

Choose a reason for hiding this comment

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

💡 для валидации ответов от внешних апи очень классно заходит использовать какую-либо из библиотек валидации json, сейчас популярна pydantic. Давай в следующем ПР попробуем ее прикрутить в этом месте.



def get_latest_posts(token: str, subreddit: str) -> list:

Choose a reason for hiding this comment

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

💡 чтобы функция была чуть более переиспользуемая, давай жесткие три дня вытащим в ее параметры (days) и будем их передавать там, где эту функцию мы используем. Оно ведь и поисследовать во время разработки проще, когда не все три дня выкачиваешь, а например, только лишь пару часов (да, пусть будет days: float)

Choose a reason for hiding this comment

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

💡 вот мы возвращаем какой-то список непонятно чего, это неудобно.

Во-первых, это реально список непонятно чего, а именно, того, что мы получили от реддит. Этот ответ в теории может меняться со временем и мы даже особо об этом не узнаем, пока в один прекрасный момент наша программа не станет падать с довольно не понятной ошибкой и не в этом методе причем. То есть причина в нем, а ошибка в другом месте - это не здорово.

Что делать? Из методов клиента в другие апишки обязательно надо возвращать что-то структурированное, вроде list[Posts], где class Post - датакласс или модель валидатора pydantic.

Во вторых, лучше использовать сразу модели валидатора, так как в этом случае, если в апи reddit что-то и измениться и произойдет ошибка - она произойдет с четким и понятным сообщением, что мы не смогли обработать ответ от реддит, так как мы ожидали там у автора поля name, а получили username вместо этого. И сразу понятно где и что править.

Поэтому, давай в следующем ПР сделаем class Post(BaseModel), пропишем какие надо поля (если надо сделаем еще дочерних классов) и вернем отсюда list[Post]

''' получить публикации субреддита за последние 3 дня '''
latest_posts = []
today = datetime.date.today()

headers = {
"Authorization": f"bearer {token}",
"User-Agent": "User-Agent"}

response = httpx.get(f"https://oauth.reddit.com/r/{subreddit}/top.json?t=week", headers=headers)

Choose a reason for hiding this comment

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

💡 этот метод давай также вытащим в клиента RedditClient

for post in response.json()['data']['children']:
created = int(post['data']['created'])
if datetime.date.fromtimestamp(created) > today - datetime.timedelta(days=3):
latest_posts.append(post)
return latest_posts


def get_authors(posts: list[dict]) -> list:

Choose a reason for hiding this comment

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

💡 не используй аннотацию dict без указания типов ключей и значений - ведь что там за ключи какие значения тому кто смотрит в функцию - непонятно, так ведь и mypy особо ничем не сможет помочь

💡 тоже самое касается возвращаемого списка list, это должен быть list[...] каких-то элементов - их тип явно должен быть обозначен

''' получить авторов публикаций '''
return [post['data']['author'] for post in posts]


def count_items_in_list(authors: list[str]) -> list[tuple[str, int]]:
''' посчитать количество публикаций авторами '''
posts_by_authors = {}

Choose a reason for hiding this comment

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

👍 хорошо, что есть тест на эту функцию
💡 тогда мы можем ее спокойно соптимизировать, используя collections.Counter

for author in authors:
if author not in posts_by_authors:
posts_by_authors[author] = 1
else:
posts_by_authors[author] = posts_by_authors[author] + 1

return list(posts_by_authors.items())


def sort_list_by_second_element(list_for_sort: list[tuple[str, int]]) -> list[tuple[str, int]]:

Choose a reason for hiding this comment

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

💡 давай тут тоже будем использовать уже имеющийся в python Counter, у него есть замечательный метод most_common

return sorted(list_for_sort, key=lambda item: item[1], reverse=True)


def get_first_items(temp_list: list[tuple[str, int]]) -> list[str]:
return [item[0] for item in temp_list]

Choose a reason for hiding this comment

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

💡 лучше эту функцию как-то поподробнее назвать, но как будто она не нужна - там где ее используешь - используй распаковку кортежа:

# например из функции get_top_authors
return [author for author, total in sorted_authors]

💡 чем меньше у нас своего кода, тем меньше нам надо тестировать, тем меньше шансов на ошибку - тем понятнее код. Это конечно не самоцель - код сокращать - но знать про такую мысль надо.



def get_top_authors(posts: list[dict]) -> list[str]:
authors = get_authors(posts)
counted_posts_by_author = count_items_in_list(authors)
sorted_authors_by_count_posts = sort_list_by_second_element(counted_posts_by_author)
return get_first_items(sorted_authors_by_count_posts)


def get_commentators(data: dict, commentators: list[str]) -> None:

Choose a reason for hiding this comment

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

💡 очень тяжело оперировать такой датой - где словарик непонятно чего, гораздо приятнее было бы сделать из этого некий датакласс или pydantic BaseModel. Давай сделаем! И на входе у нас будет что-то понятное, а то сейчас даже не сразу понимаю что это - пост из ответа апишки?

for comment_data in data['data']['children']:
author = comment_data['data']['author']
commentators.append(author)
if comment_data['data']['replies']:
get_commentators(comment_data['data']['replies'], commentators)


def get_comments(token: str, post_id: str) -> dict:

Choose a reason for hiding this comment

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

💡 это тоже надо вынести в наш класс RedditClient
💡 метод клиента к апи не должен возвращать непонятный дикт, он должен возвращать что-то конкретное - тут это список комментариев - это должен быть: list[Comment]

headers = {
"Authorization": f"bearer {token}",
"User-Agent": "ChangeMeClient/0.1 by YourUsername"}

Choose a reason for hiding this comment

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

💡 имя UserAgent должен быть твой аккаунт и если ты не хочешь шарить его на github - сделай себе .env файл переменных окружения, и запускайся с ним.

💡 также нам понадобиться некоторый модуль config.py с датаклассом Config, в котором мы будем иметь все что нам надо, имена явки пароли и прочие конфиг параметры которые мы считаем с помощью os.environ из переменных окружения


response = httpx.get(f"https://oauth.reddit.com/r/{subreddit}/comments/{post_id}/comment.json", headers=headers)
return response.json()[1]


def get_top_commentators(token: str, posts: list) -> list:

Choose a reason for hiding this comment

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

💡 обязательно указывай, а что же там за типы у нас в list - какие это элементы
💡 и конечно, нам хотелось бы, что это были posts: list[Post], а в ответе что? непонятно... список строк?

commentators: list[str] = []

for post in posts:
post_id = post['data']['id']
comments = get_comments(token, post_id)
get_commentators(comments, commentators)

counted_comments_by_author = count_items_in_list(commentators)
sorted_authors_by_count_comments = sort_list_by_second_element(counted_comments_by_author)
return get_first_items(sorted_authors_by_count_comments)


def main():

Choose a reason for hiding this comment

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

👍 отличный main, все максимально лаконично и понятно

token = get_token(client_id, client_secret, username, password)
posts = get_latest_posts(token, subreddit)
top_authors = get_top_authors(posts)
top_commentators = get_top_commentators(token, posts)

print(top_authors)
print(top_commentators)


if __name__ == '__main__':
main()

Choose a reason for hiding this comment

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

❗ как раз пример того файла, который должен был быть исключен при отправке в github. Давай его удалим следующим образом:

  • git rm tests/__pycache__/test_main.cpython-312-pytest-8.0.2.pyc
  • после чего сделаем git commit -m "remove pycache file"
  • потом добавим все __pycache__ файлы в .gitignore файл:
__pycache__

Binary file not shown.
101 changes: 101 additions & 0 deletions tests/test_main.py
Original file line number Diff line number Diff line change
@@ -0,0 +1,101 @@
import pytest
import httpx

from sensitive_data import client_id, client_secret, username, password

Choose a reason for hiding this comment

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

💡 вместо заигноренного sensitive_data.py используй config.py, который ты положешь репозиторий спокойно без твоих кредов, и используй заигноренный .env. Обязательно сделай .env.template без реальных секретов, но с названиями переменных и может быть их описанием в комментах, который ты должен также коммитнуть в репозиторий для тех, кто хочет запустить твою программу, но уже со своими секретами.

💡 А про вариант с заигноренным py файлом можем спокойно забыть навсегда.

💡 хотя наши unit тесты не должны знать ничего про реальные твои креды в реддит - они не должны туда ходить в принципе.

from main import (get_token, get_latest_posts, get_authors, count_items_in_list, sort_list_by_second_element,
get_top_authors, get_first_items, get_comments)


@pytest.fixture
def token():
return get_token(client_id, client_secret, username, password)

Choose a reason for hiding this comment

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

💡 не используй свои реальные креды в тестах - мокай реальные запросы в reddit с помощью либы respx



@pytest.fixture
def subreddit():
return 'learnpython'


@pytest.fixture
def posts(token, subreddit):

Choose a reason for hiding this comment

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

❗ фикстура не должна использовать тестируемые функции (тестируемая функция может быть использована только лишь в своем собственном тесте на нее)

❗ фикстуры не должны создаваться с реальными токенами и реальными запросами в чужие сервисы (в нашем случае reddit). Нужно вернуть просто список постов - сделать их надо самому. Хочешь прочитай их из лежащего рядом json файла. А лучше сделать фикстуру-фабрику, которая генерит такой словарь

return get_latest_posts(token, subreddit)


def test__get_token__returns_string():

Choose a reason for hiding this comment

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

❗ тесты, которые тестируют, тип возвращаемого значения не нужны - выпиливай. Этим занимается mypy - его запускают для этого.

assert isinstance(get_token(client_id, client_secret, username, password), str)


def test__get_token__returns_exception_with_blank_client_id():

Choose a reason for hiding this comment

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

💡 вот если сделаешь config.py и класс RedditClient необходимость в подобных тестах отпадет и можно будет больше сил уделить именно тестам на логику.

with pytest.raises(KeyError):
get_token(client_id='', client_secret=client_secret, username=username, password=password)


def test__get_token__returns_exception_with_blank_client_secret():
with pytest.raises(KeyError):
get_token(client_id=client_id, client_secret='', username=username, password=password)


def test__get_token__returns_exception_with_blank_username():
with pytest.raises(KeyError):
get_token(client_id=client_id, client_secret=client_secret, username='', password=password)


def test__get_token__returns_exception_with_blank_password():
with pytest.raises(KeyError):
get_token(client_id=client_id, client_secret=client_secret, username=username, password='')


def test__get_latest_posts__returns_list(token, subreddit):

Choose a reason for hiding this comment

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

❗ выпиливай - тест на mypy нам не нужен

assert isinstance(get_latest_posts(token, subreddit), list)


def test__get_latest_posts__returns_exception_with_blank_token(subreddit):

Choose a reason for hiding this comment

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

💡 не имеем права ходить в реальный реддит в таких тестах - надо сделать клиента, использовать затем respx для каких то подобных тестах. Но не то, что райзит ошибку - нет, у тебя ситуации не должно быть, что ты с пустым токеном бежишь (а это можно сделать как раз в каком нить config.py)

А тут был бы тест, что в клиенте при запросе в последние посты - передается нужный токен в нужном поле.

with pytest.raises(httpx.LocalProtocolError):
get_latest_posts(token='', subreddit=subreddit)


def test__get_latest_posts__returns_exception_with_blank_subreddit(token):
with pytest.raises(KeyError):
get_latest_posts(token=token, subreddit='')

Choose a reason for hiding this comment

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

💡 лучше исключать до попадания в клиент запрос на пустой subreddit. Где-то в момент запроса от пользователя этого самого subreddit, дальше считать, что он везде валидный. Это довольно важный момент обработки внештатных ситуаций - их нужно не размазывать по всей программе - а обрабатывать в том самом месте, где она может возникнуть.



def test__get_authors__returns_authors_list():

Choose a reason for hiding this comment

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

👍 вот это уже ближе к реальности тест - хороший, единственное только название - нам же не важно тут что возвращается list - его можно не писать (за типы mypy работает), то есть return_authors было бы достаточно

data = [{'data': {'author': 'author1'}},
{'data': {'author': 'author2'}}]
assert get_authors(data) == ['author1', 'author2']


@pytest.mark.parametrize(('data', 'expected_result'), [
([{'data': {'author': 'author1'}}], ['author1']),
([{'data': {'author': 'author1'}}, {'data': {'author': 'author2'}}], ['author1', 'author2'])
])
def test__get_authors__returns_authors_list(data, expected_result):

Choose a reason for hiding this comment

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

❗ вижу два теста с одинаковым названием - непорядок. Кейс один получается? В чем особенность параметров? Не лучше ли иметь два разных теста с очевидным названием?

  • can_return_one_author
  • can_return_many_authors

И тесты станут понятнее и запутанные сложные параметры будут не нужны.

💡 то есть я это к чему, параметры зачастую плохо применять, чем хорошо. Должен быть очень веский повод в пользу параметров. Как минимум они в этом случае должны быть максимально простыми и ясными и не касаться разных кейсов.

assert get_authors(data) == expected_result


@pytest.mark.parametrize(('items', 'expected_result'), [

Choose a reason for hiding this comment

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

👍 хороший тест ведь
💡 но параметры вообще мимо - что и видно по названию теста - что он тестит - само название не отражает - а обязано. А все потому что кейсы разные. Вот.

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

(['author'], [('author', 1)]),
(['author', 'author'], [('author', 2)]),
(['author1', 'author2'], [('author1', 1), ('author2', 1)]),
])
def test__count_items_in_list__returns_count_items(items, expected_result):
assert count_items_in_list(items) == expected_result


@pytest.mark.parametrize(('items', 'expected_result'), [

Choose a reason for hiding this comment

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

💡 этого и ниже тестов постигла та же участь, что и тест выше - параметры лишние - кейсы - разные - а значит разные должны быть тесты - не экономь ты их.

([], []),
([('author', 1), ('author', 1)], [('author', 1), ('author', 1)]),
([('author', 1), ('author', 2)], [('author', 2), ('author', 1)]),
])
def test__sort_list_by_second_element__returns_sorted_items(items, expected_result):
assert sort_list_by_second_element(items) == expected_result


@pytest.mark.parametrize(('tuples_list', 'expected_result'), [
([], []),
([('author1', 2), ('author2', 1)], ['author1', 'author2']),
])
def test__get_first_items__returns_first_items(tuples_list, expected_result):
assert get_first_items(tuples_list) == expected_result