Skip to content
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.

Sign up for GitHub

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

Jump to bottom

Nenuzhny Andrey #54

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

Nenuzhny Andrey #54

wants to merge 89 commits into from

Conversation

Nenu1985
Copy link

@Nenu1985 Nenu1985 commented Nov 16, 2019 •
edited
Loading

Finished SECOND iteration

Andrey Nenuzhny added 11 commits November 10, 2019 16:40
Init commit of first task
5d63c1d
Base and yahoo classes are comleted
21e3713 
Works --limit snd --json argument processing for yahoo rss url
Added tut and default bots classes
ded173c
Remade whole architecture
cb1c1b6 
Rss readers splitted into default and particual to specific url.
Implemented Yahoo and Tut.by url specific bots.

Chosing an appropriate bot is made by dynamic module importing.

News out is implemented as table view.
Begin tests implementation
e61350b
Cod is covered with test over 90%
a32fcda
readme file with project description
42e239d
Script to check code checks using pycodestyle module
d155454 
pycodestyle module must to be installed
First iteration is completed
f2606f8
gitignore update after merging into remote repo
1b4df02
init second iteration commit - create a draft of setup.py
c3fb53a
@Nenu1985 Nenu1985 changed the title Late PR [WIP] Nenuzhny Andrey Nov 17, 2019
@AlexeiBuzuma AlexeiBuzuma added the MissedPRDeadline Pull request has been opened after hard deadline. label Nov 17, 2019
@Nenu1985
Copy link
Author

Updated with implemented distribution

@AlexeiBuzuma AlexeiBuzuma added the [Deadline] Iteration 1-2 This is a marker for first and second iterations deadline. label Nov 17, 2019
Andrey Nenuzhny and others added 7 commits November 18, 2019 15:03
Restructured news to pass mypy cheks.
1d42004 
- Default bot is ready
- Yahoo bot is ready
- Tut bot is ready
- Test are done
Restructured news to pass mypy cheks.
8d8fe31 
- Default bot is ready
- Yahoo bot is ready
- Tut bot is ready
- Test are done
@Nenu1985
Merge pull request #1 from Nenu1985/fix/structured-news
e184bdd 
Restructured news to pass mypy cheks.
Merge branch 'fix/structured-news' of github.com:Nenu1985/PythonHomew…
335b165 
…ork into fix/structured-news
Merge branch 'fix/structured-news'
59806d7
Some mypy`s annotations optimization
c78bcae
Implement feature to store news via decorators, json and pickle
25d9379
@Nenu1985 Nenu1985 changed the title [WIP] Nenuzhny Andrey Nenuzhny Andrey 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. Подробный readme файл
  2. sh скрипты для установки/запуска решения -- это всегда упращает жизнь коллегам по цеху
  3. сделана иерархия исключений
  4. свой конфиг для mypy

Моменты, которые можно улучшить:

  1. В коде есть raw sql и жесткая привязка к SQLlite. Я думаю, что для проекта такого размера это в целом нормально, но raw sql в пайтон коде почти всегда выглядит немного инородным
  2. Именование ботов должно быть более конкретным. Да, мы можем понять суть из названия модулей (tut.py, yahoo.py ...), но если эти классы импортируются в какой-то модуль, то там могут случайно возникнуть конфликты имен, либо просто можно будет легко запутаться в коде. В данном случае эти боты импортируются динамически и действительно сложно понять, какая конкретная реализация класса тут будет использоваться.
  3. Дефолтная имплементация бота не содержит никакой реализации. Не совсем понятно, зачем этот класс существует.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@AlexeiBuzuma AlexeiBuzuma AlexeiBuzuma left review comments

At least 1 approving review is required to merge this pull request.

Assignees
No one assigned
Labels
MissedPRDeadline Pull request has been opened after hard deadline. Reviewed [1-2 iteration] Minimal requirements checked [3-4 iteration] Minimal requirements checked
Projects
None yet
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
2 participants
@Nenu1985 @AlexeiBuzuma