-
Notifications
You must be signed in to change notification settings - Fork 0
First #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
base: master
Are you sure you want to change the base?
First #1
Changes from all commits
File filter
Filter by extension
Conversations
Jump to
Diff view
Diff view
There are no files selected for viewing
This file was deleted.
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 👍 круто, что используешь |
||
| from sensitive_data import client_id, client_secret, username, password | ||
|
|
||
| subreddit = 'learnpython' | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 💡 мы не пишим скрипты - мы пишем программы, приложения, сервисы - поэтому никаких глобальных переменных 💡 давай будем спрашивать о том сабреддите, который хотим спарсить у пользователя с помощью |
||
|
|
||
|
|
||
| def get_token(client_id: str, client_secret: str, username: str, password: str) -> str: | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 👍 хороший метод и аннотации верно проставлены Поэтому, давай все функция, где используется |
||
| ''' получить токен ''' | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 💡 для |
||
| post_data = {"grant_type": "password", "username": username, "password": password} | ||
| headers = {"User-Agent": "User-Agent"} | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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'] | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 💡 для валидации ответов от внешних апи очень классно заходит использовать какую-либо из библиотек валидации |
||
|
|
||
|
|
||
| def get_latest_posts(token: str, subreddit: str) -> list: | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 💡 чтобы функция была чуть более переиспользуемая, давай жесткие There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 💡 вот мы возвращаем какой-то список непонятно чего, это неудобно. Во-первых, это реально список непонятно чего, а именно, того, что мы получили от реддит. Этот ответ в теории может меняться со временем и мы даже особо об этом не узнаем, пока в один прекрасный момент наша программа не станет падать с довольно не понятной ошибкой и не в этом методе причем. То есть причина в нем, а ошибка в другом месте - это не здорово. Что делать? Из методов клиента в другие апишки обязательно надо возвращать что-то структурированное, вроде Во вторых, лучше использовать сразу модели валидатора, так как в этом случае, если в апи reddit что-то и измениться и произойдет ошибка - она произойдет с четким и понятным сообщением, что мы не смогли обработать ответ от реддит, так как мы ожидали там у автора поля name, а получили username вместо этого. И сразу понятно где и что править. Поэтому, давай в следующем ПР сделаем class |
||
| ''' получить публикации субреддита за последние 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) | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 💡 этот метод давай также вытащим в клиента |
||
| 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: | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 💡 не используй аннотацию 💡 тоже самое касается возвращаемого списка |
||
| ''' получить авторов публикаций ''' | ||
| return [post['data']['author'] for post in posts] | ||
|
|
||
|
|
||
| def count_items_in_list(authors: list[str]) -> list[tuple[str, int]]: | ||
| ''' посчитать количество публикаций авторами ''' | ||
| posts_by_authors = {} | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 👍 хорошо, что есть тест на эту функцию |
||
| 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]]: | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 💡 давай тут тоже будем использовать уже имеющийся в python Counter, у него есть замечательный метод |
||
| 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] | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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: | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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: | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 💡 это тоже надо вынести в наш класс |
||
| headers = { | ||
| "Authorization": f"bearer {token}", | ||
| "User-Agent": "ChangeMeClient/0.1 by YourUsername"} | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 💡 имя 💡 также нам понадобиться некоторый модуль |
||
|
|
||
| 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: | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 💡 обязательно указывай, а что же там за типы у нас в |
||
| 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(): | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 👍 отличный |
||
| 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() | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ❗ как раз пример того файла, который должен был быть исключен при отправке в
__pycache__ |
| 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 | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 💡 вместо заигноренного 💡 А про вариант с заигноренным 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) | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 💡 не используй свои реальные креды в тестах - мокай реальные запросы в reddit с помощью либы |
||
|
|
||
|
|
||
| @pytest.fixture | ||
| def subreddit(): | ||
| return 'learnpython' | ||
|
|
||
|
|
||
| @pytest.fixture | ||
| def posts(token, subreddit): | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ❗ фикстура не должна использовать тестируемые функции (тестируемая функция может быть использована только лишь в своем собственном тесте на нее) ❗ фикстуры не должны создаваться с реальными токенами и реальными запросами в чужие сервисы (в нашем случае |
||
| return get_latest_posts(token, subreddit) | ||
|
|
||
|
|
||
| def test__get_token__returns_string(): | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ❗ тесты, которые тестируют, тип возвращаемого значения не нужны - выпиливай. Этим занимается |
||
| assert isinstance(get_token(client_id, client_secret, username, password), str) | ||
|
|
||
|
|
||
| def test__get_token__returns_exception_with_blank_client_id(): | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 💡 вот если сделаешь |
||
| 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): | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ❗ выпиливай - тест на |
||
| assert isinstance(get_latest_posts(token, subreddit), list) | ||
|
|
||
|
|
||
| def test__get_latest_posts__returns_exception_with_blank_token(subreddit): | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 💡 не имеем права ходить в реальный реддит в таких тестах - надо сделать клиента, использовать затем А тут был бы тест, что в клиенте при запросе в последние посты - передается нужный токен в нужном поле. |
||
| 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='') | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 💡 лучше исключать до попадания в клиент запрос на пустой |
||
|
|
||
|
|
||
| def test__get_authors__returns_authors_list(): | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. 👍 вот это уже ближе к реальности тест - хороший, единственное только название - нам же не важно тут что возвращается list - его можно не писать (за типы mypy работает), то есть |
||
| 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): | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe reason will be displayed to describe this comment to others. Learn more. ❗ вижу два теста с одинаковым названием - непорядок. Кейс один получается? В чем особенность параметров? Не лучше ли иметь два разных теста с очевидным названием?
И тесты станут понятнее и запутанные сложные параметры будут не нужны. 💡 то есть я это к чему, параметры зачастую плохо применять, чем хорошо. Должен быть очень веский повод в пользу параметров. Как минимум они в этом случае должны быть максимально простыми и ясными и не касаться разных кейсов. |
||
| assert get_authors(data) == expected_result | ||
|
|
||
|
|
||
| @pytest.mark.parametrize(('items', 'expected_result'), [ | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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'), [ | ||
|
There was a problem hiding this comment. Choose a reason for hiding this commentThe 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 | ||
|
|
||
|
|
||
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
❗ этот файл крайне полезный - не удаляй его, он позволяет нам понимать, какие файлы подлежат версионированию и отправке в гитхаб, а какие исключены